Albrecht Schlosser wrote:
> Well, we *have* a patch, and I think that it is so far okay, but
> I'd like to have someone else check it. Ian, Greg, could you maybe
> take a look at it? There are two different patches in STR 2405.
> Any comments welcome. I'd like to commit the patches, but not before
> the next monday, so there's some time left to comment... ;-)
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.
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).
Here's a small test program that simulates 3 different values for
'align()',
showing the value results. The test shows 4 results:
original code, Boris's technique, Albrecht's technique, and what I'd
suggest.
Each result shows 3 variations on the value of align(): 0x00, 0x04, and
0x0f.
#include <stdio.h>
int main()
{
{ char al = (0x00 & 0x04 != 0); printf(" orig al[00]=%d\n", al); }
{ char al = (0x04 & 0x04 != 0); printf(" orig al[04]=%d\n", al); }
{ char al = (0x0f & 0x04 != 0); printf(" orig al[0f]=%d\n", al); }
printf("\n");
{ char al = (0x00 & (0x04 != 0)); printf(" boris al[00]=%d\n", al); }
{ char al = (0x04 & (0x04 != 0)); printf(" boris al[04]=%d\n", al); }
{ char al = (0x0f & (0x04 != 0)); printf(" boris al[0f]=%d\n", al); }
printf("\n");
{ char al = (0x00 & 0x04); printf("albrecht al[00]=%d\n", al); }
{ char al = (0x04 & 0x04); printf("albrecht al[04]=%d\n", al); }
{ char al = (0x0f & 0x04); printf("albrecht al[0f]=%d\n", al); }
printf("\n");
{ char al = ((0x00 & 0x04) != 0); printf(" greg al[00]=%d\n", al); }
{ char al = ((0x04 & 0x04) != 0); printf(" greg al[04]=%d\n", al); }
{ char al = ((0x0f & 0x04) != 0); printf(" greg al[0f]=%d\n", al); }
}
..here's the output when run:
orig al[00]=0
orig al[04]=0
orig al[0f]=1 <--
boris al[00]=0
boris al[04]=0
boris al[0f]=1 <--
albrecht al[00]=0
albrecht al[04]=4 <--
albrecht al[0f]=4 <--
greg al[00]=0
greg al[04]=1 <--
greg al[0f]=1 <--
Note that boris's patch simulates the original code's results.
Albrecht and my suggested patches appear to modify the original
behavior of the code, but may be more what the original code wants.
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).
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.
_______________________________________________
fltk-dev mailing list
[email protected]
http://lists.easysw.com/mailman/listinfo/fltk-dev
