Issue 100502 - oox: gcc finds operations && || ambigous
Summary: oox: gcc finds operations && || ambigous
Status: CLOSED FIXED
Alias: None
Product: xml
Classification: Code
Component: code (show other issues)
Version: DEV300m44
Hardware: All Linux, all
: P3 Trivial (vote)
Target Milestone: OOo 3.2
Assignee: daniel.rentz
QA Contact: issues@xml
URL:
Keywords:
Depends on:
Blocks: 96084
  Show dependency tree
 
Reported: 2009-03-24 09:59 UTC by caolanm
Modified: 2009-09-22 09:09 UTC (History)
3 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
version 1, keeps current behaviour (476 bytes, patch)
2009-03-24 10:02 UTC, caolanm
no flags Details | Diff
version 2, there's a chance that this is what was meant (476 bytes, patch)
2009-03-24 10:03 UTC, caolanm
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description caolanm 2009-03-24 09:59:35 UTC
In oox/source/docprop/docprophandler.cxx there is...

if ( m_nInBlock == 2 || m_nInBlock == 3 && m_nType )

I want to confirm that the operator precedence is intentional, i.e.
we mean as version1 patch attached, and not that we instead meant like version2
Comment 1 caolanm 2009-03-24 10:02:11 UTC
Created attachment 61137 [details]
version 1, keeps current behaviour
Comment 2 caolanm 2009-03-24 10:03:07 UTC
Created attachment 61139 [details]
version 2, there's a chance that this is what was meant
Comment 3 daniel.rentz 2009-03-24 10:20:49 UTC
dr->mav: I assume the second option is what you meant, right? I will add this to
my CWS.
Comment 4 daniel.rentz 2009-03-24 10:26:38 UTC
grabbing issue...
Comment 5 daniel.rentz 2009-03-24 10:28:18 UTC
fixed in CWS DEV300/dr68 (OOo 3.2)
Comment 6 mikhail.voytenko 2009-03-24 10:37:38 UTC
mav->dr: No, the source code is so as it was intended. The second patch will
break the implementation. If you would like to introduce the patch, please use
the first one that does not change the behavior.

PS: I assume that the change is done because of the new warning of the gcc
compiler. Although we have to support it, from my point of view the warning is
more than just useless.
 
Comment 7 mikhail.voytenko 2009-03-24 10:40:35 UTC
Changing the title
Comment 8 daniel.rentz 2009-03-24 10:43:47 UTC
changed implementation accordingly, thanks for your feedback.
Comment 9 caolanm 2009-03-24 10:51:43 UTC
FWIW, while in this case the implementation was correct, in about 40 out of 45
previous cases the precedence rules were not what was intended so it would
appear to be a quite useful warning. Though as we're down to the small subset of
gray ones where it the code is neither obviously wrong or obvious right I guess
the ratio will change
Comment 10 Mathias_Bauer 2009-03-24 14:21:33 UTC
Mikhail, I also thought at first that these kinds of warnings are superfluous.
But I learned and saw that they revealed some bugs in other places, where
(different to your case) the code was not working as the sloppy developer
wanted. So it makes a lot of sense to set braces even if they are not necessary.
It also makes the code easier to read and understand, and that's a good thing also.
Comment 11 mikhail.voytenko 2009-03-24 15:21:28 UTC
If the warning has helped to find and fix around 40 bugs from 45 suspicious
cases, it indeed makes sense. 

I did not pretend to be error prove in case of logical expressions. It is
probably a matter of taste, for me additional brackets, especially brackets in
brackets, make in most cases big expressions less readable.
Comment 12 daniel.rentz 2009-05-15 17:17:01 UTC
verified in CWS
Comment 13 daniel.rentz 2009-09-22 09:09:47 UTC
closed