Re: [PATCHv2 0/3] catch non-sign-extended '~' brainos

[Phil Carmody <phil@dovecot.fi>]

On Tue, Jul 01, 2014 at 12:42:51PM -0700, Christopher Li wrote:
> On Tue, Jul 1, 2014 at 4:30 AM, Phil Carmody <phil@dovecot.fi> wrote:
> > With unsigned, it becomes a lot quieter, but still much noiser than my
> > original. (I have a v3 which excludes an idiom using '/'.)
> 
> Yes, because it cover more than just binary operations.
> If you consider zero extend the unsigned type introducing error,
> the binary operation just how you use the error bits.
> 
> >> #define NLMSG_ALIGNTO    4U
> >
> > I consider that, pedantically, to be worth fixing. alignments are defined
> > by the C standard to be of type size_t. It should either be ((size_t)4)
> 
> ((size_t)4) is hell ugly.

Do you care about the precise type of the value?
If you do, then it may be ugly, but it's pretty much obligatory.
And if you don't care about the precise type...

> > if you care about the precise type, or 4 if you don't, but not 4U. The
> > 'U' adds nothing apart from a way to create an incorrect mask.
> 
> That is debatable. Enforcing 4U vs 4 is annoying and buy you nothing
> most of the case.

What do you mean by "enforcing"? I consider persuading people to write
simpler, less breakable code to be a good thing.

> I don't consider alignment being 4U as wrong by itself. The key question
> is how you use it.

Absolutely. And doing a ~ and then an up-cast is using it dangerously.
Isn't that what sparse is supposed to be checking for?

> if we introduce:
> 
> #define ALIGNMASK(align)   (size_t)(~((align) -1))
> 
> And we always use the macro to create mask from alignment value.
> Then there is no need to enforce signedness of the var passed into the
> macro. Hey, it might even read better.

Put 4U in that, and it still has the flaw of creating a truncated mask.
Put 4 in and it doesn't.

Do you still want to defend 4U?

Phil