something to warry about...

[Denys Vlasenko]

On Wed, Jan 19, 2011 at 1:20 PM, Johannes Stezenbach <js at sig21.net> wrote:
>> >  static smallint detect_link_mii(void)
>> >  {
>> > -       struct ifreq ifreq;
>> > -       struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;
>> > +       union mii_ifreq mii_ifreq;
>> > +       union mii_ifreq *mii = (void *)&mii_ifreq.ifreq.ifr_data;
>>
>> Here you again assign void** to union mii_ifreq* -
>> types still do not match.
>
> That's why I suggested to replace ifr_data with ifr_ifru,
> which is less confusing but the result is the same.

No, it should not be changed.

We do not write for compiler. We write for *humans*
to read our code.

ifr_data isn't called "data" for no reason. It means
"a generic data field", and here we use it exactly
for that purpose.

Whereas ifr_ifru means "ifr union" and is only
an implementation detail of the  struct ifreq,
not meant to be used directly.
Had C supported unnamed members like C++ does,
struct ifreq would have that member union without name.


>> This doesn't emit warnings probably because gcc
>> takes a brute-force approach to handling unions
>> by marking every pointer derived from any union member
>> "potentially aliases anything". Future versions of gcc
>> may become "more clever" again, and warnings will return.
>
> I disagree.  C99 aliasing rules say two pointers of the
> same type can alias.  Consider the follwing:
>
>        void *p = malloc(1000);
>        union mii_ifreq *m1 = p;
>        union mii_ifreq *m2 = p + 8;
>
> m1 and m2 can alias, and this does not change if you
> change the line to
>
>        union mii_ifreq *m2 = (void *)&m1->ifreq.ifr_ifru;

>> > +       union mii_ifreq mii_ifreq;
>> > +       union mii_ifreq *mii = (void *)&mii_ifreq.ifreq.ifr_data;

In your patch, the pointers have an offset relative each other:
even if compiler will assume they are aliased, aliasing means
mii_ifreq.foo and mii->foo need to be considered as possibly the same.
But in your case, mii_ifreq.foo doesn't map to mii->foo,
it maps to mii->bar!

But I have a deeper problem here. I do not see it as a good decision
to butcher a readable piece of code into a obfuscated mess *only*
to shut up gcc. gcc has to provide means to do it in some cleaner way.

-- 
vda