On 25.09.2010, at 17:28, Greg Ercolano wrote: [regarding STR #2405]
> The suggested Fl_Scroll patches are interesting to me: > > Boris Fl_Scroll patch: > - char al = (scrollbar.align()&FL_ALIGN_LEFT!=0); > - char at = (scrollbar.align()&FL_ALIGN_TOP!=0); > + char al = (scrollbar.align()&(FL_ALIGN_LEFT!=0)); > + char at = (scrollbar.align()&(FL_ALIGN_TOP!=0)); > > Albrecht Fl_Scroll patch: > - char al = (scrollbar.align()&FL_ALIGN_LEFT!=0); > - char at = (scrollbar.align()&FL_ALIGN_TOP!=0); > + char al = scrollbar.align()&FL_ALIGN_LEFT; > + char at = scrollbar.align()&FL_ALIGN_TOP; > > The C++ rules of precedence > http://www.cppreference.com/wiki/operator_precedence > says that != (9) takes precedence over '&' (10), which seems to imply > that Boris's > patch is technically correct. > > And in actual practice, I think it's equivalent to the actual > functioning of the code as it stands pre-patched. Yep, you're right. > However, I /think/ the intention of the code is for the& > to take precedence. (ie. we may have found a bug in the original code). True. That's why I was confused and thought that Boris's *patch* was wrong. In fact, he (Boris) was correct, but: taking the correct precedences, ... (FL_ALIGN_TOP!=0) == (FL_ALIGN_LEFT!=0) == 1 (true), and hence the original code evaluates to: char al = (scrollbar.align()&1); char at = (scrollbar.align()&1); That's not what was intendend. But in the "usual" case (scrollbars at the bottom and right side), both expressions evaluate to 0, so that they were "usually right". > Here's a small test program[...] > albrecht al[00]=0 > albrecht al[04]=4<-- > albrecht al[0f]=4<-- [...] > Albrecht, I think the !=0 is a good thing to preserve (I see you > removed it), because it limits the result to a true/false value > that can fit in a char. As you can see from the above, the untamed > result can be>1, and thus, potentially the value could be larger > than could fit in a char (eg. 0x0100 for instance). You're right. I overlooked the "char" at the left hand side. :-( My first patch (for 'al' only) was something like: - char al = (scrollbar.align()&FL_ALIGN_LEFT!=0); + char al = (scrollbar.align()&FL_ALIGN_LEFT)!=0; or + char al = ((scrollbar.align()&FL_ALIGN_LEFT)!=0); but then I removed the "!=0" because it seemed redundant to me, not realizing that Fl_Align is an unsigned int. [However, taken the real values of FL_ALIGN_LEFT (4) and FL_ALIGN_TOP (1), it would have been always correct.] > I'd suggest Boris's code if the original pre-patched code is correctly > intentioned and not a bug. Otherwise, I'd suggest the following which > would keep the true/false value, and 'fix' the intention of the code: > > - char al = (scrollbar.align()&FL_ALIGN_LEFT!=0); > - char at = (scrollbar.align()&FL_ALIGN_TOP!=0); > + char al = (scrollbar.align()&FL_ALIGN_LEFT)!=0; > + char at = (scrollbar.align()&FL_ALIGN_TOP)!=0; > > Comments welcome. I think the best solution is your above proposal with the additional parens you mentioned later. This keeps the code mostly untouched and adds only the needed parentheses: - char al = (scrollbar.align()&FL_ALIGN_LEFT!=0); - char at = (scrollbar.align()&FL_ALIGN_TOP!=0); + char al = ((scrollbar.align()&FL_ALIGN_LEFT)!=0); + char at = ((scrollbar.align()&FL_ALIGN_TOP)!=0); This should be the correct and *intended* solution. I'll change my proposed patch in STR 2405 accordingly. Thanks for finding this. ----- BTW, that's what the compiler warning was intended for. The following is from the gcc 4.5.0 manual: -Wparentheses Warn if parentheses are omitted in certain contexts, such as ... or when operators are nested whose precedence people often get confused about. Albrecht P.S.: To all: please have a look at the other patches in STR 2405. I'd really appreciate if someone else checked the patches. Maybe there are more hidden bugs. Thanks. _______________________________________________ fltk-dev mailing list [email protected] http://lists.easysw.com/mailman/listinfo/fltk-dev
