On 2016.09.29 at 18:52 +0000, Bernd Edlinger wrote:
> On 09/29/16 20:03, Jason Merrill wrote:
> > On Wed, Sep 28, 2016 at 12:09 PM, Bernd Edlinger
> > <bernd.edlin...@hotmail.de> wrote:
> >> On 09/28/16 16:41, Jason Merrill wrote:
> >>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger
> >>> <bernd.edlin...@hotmail.de> wrote:
> >>>> On 09/27/16 16:42, Jason Merrill wrote:
> >>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger
> >>>>> <bernd.edlin...@hotmail.de> wrote:
> >>>>>> On 09/27/16 16:10, Florian Weimer wrote:
> >>>>>>> * Bernd Edlinger:
> >>>>>>>
> >>>>>>>>> “0 << 0” is used in a similar context, to create a zero constant
> >>>>>>>>> for a
> >>>>>>>>> multi-bit subfield of an integer.
> >>>>>>>>>
> >>>>>>>>> This example comes from GDB, in bfd/elf64-alpha.c:
> >>>>>>>>>
> >>>>>>>>> | insn = INSN_ADDQ | (16 << 21) | (0 << 16) | (0 << 0);
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Of course that is not a boolean context, and will not get a warning.
> >>>>>>>>
> >>>>>>>> Question is if "if (1 << 0)" is possibly a miss-spelled "if (1 < 0)".
> >>>>>>>>
> >>>>>>>> Maybe 1 and 0 come from macro expansion....
> >>>>>>>
> >>>>>>> But what's the intent of treating 1 << 0 and 0 << 0 differently in the
> >>>>>>> patch, then?
> >>>>>>
> >>>>>> I am not sure if it was a good idea.
> >>>>>>
> >>>>>> I saw, we had code of the form
> >>>>>> bool flag = 1 << 2;
> >>>>>>
> >>>>>> another value LOOKUP_PROTECT is 1 << 0, and
> >>>>>> bool flag = 1 << 0;
> >>>>>>
> >>>>>> would at least not overflow the allowed value range of a boolean.
> >>>>>
> >>>>> Assigning a bit mask to a bool variable is still probably not what was
> >>>>> intended, even if it doesn't change the value.
> >>>>
> >>>> That works for me too.
> >>>> I can simply remove that exception.
> >>>
> >>> Sounds good.
> >>
> >> Great. Is that an "OK with that change"?
> >
> > What do you think about dropping the TYPE_UNSIGNED exception as well?
> > I don't see what difference that makes.
> >
>
>
> If I drop that exception, then I could also drop the check for
> INTEGER_TYPE and the whole if, because I think other types can not
> happen, but if they are allowed they are as well bogus here.
>
> I can try a bootstrap and see if there are false positives.
>
> But I can do that as well in a follow-up patch, this should probably
> be done step by step, especially when it may trigger some false
> positives.
>
> I think I could also add more stuff, like unary + or - ?
> or maybe also binary +, -, * and / ?
>
> We already discussed making this a multi-level option,
> and maybe enabling the higher level explicitly in the
> boot-strap.
>
> As long as the warning continues to find more bugs than false
> positives, it is probably worth extending it to more cases.
>
> However unsigned integer shift are not undefined if they overflow.
>
> It is possible that this warning will then trigger also on valid
> code that does loop termination with unsigned int left shifting.
> I dont have a real example, but maybe like this hypothetical C-code:
>
> unsigned int x=1, bits=0;
> while (x << bits) bits++;
> printf("bits=%d\n", bits);
>
>
> Is it OK for everybody to warn for this on -Wall, or maybe only
> when -Wextra or for instance -Wint-in-bool-context=2 is used ?
I'm seeing this warning a lot in valid low level C code for unsigned
integers. And I must say it look bogus in this context. Some examples:
return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
if ( (uint32_t) ( aSig<<( shiftCount & 31 ) ) ) {
&& (uint64_t) (extractFloatx80Frac(a) << 1))
if ((plen < KEYLENGTH) && (key << plen))
--
Markus
