Re: [PATCH v2] Implement pahole-like 'ptype /o' option

[Pedro Alves <palves at redhat dot com>]

On 12/12/2017 12:25 AM, Sergio Durigan Junior wrote:
> On Monday, December 11 2017, Pedro Alves wrote:

>> IMO, the right way to go about this is to decide on a format,
>> document it in the wiki and then we can point everyone at it with a URL.
>> (ISTR that GCC's coding conventions documents yet another
>> way to break the line, and in that case, not even GCC follows it.)
>>
>> IMO, the format you've chosen isn't ideal because it requires
>> manual right-alignment for every line, while breaking before the
>> parens makes emacs's TAB automatically align the following lines.
>> The latter also gives a lot more space for the params again; it's
>> sort of a "well, I have to do something, so might as well start
>> way back from the left again).
> 
> No, it's not ideal at all.  But at least on my Emacs, the other format
> has the downside of being always realigned to the column zero when I
> press TAB on it. But again, I'm not saying this is Emacs fault nor your
> nor Simon's fault; this is just something I noticed.

Seems easier to fix (just two spaces, and only on one line) than
the other approach, which depends a varying number of
space/delete strokes.  Maybe there's a way to configure emacs
not to do that, even?

> 
>>>>>> But I don't mind it, it just stuck out as a little inconsistency.
>>>>>
>>>>> I don't see the inconsistency.
>>>>>
>>>>> If a field is inside a struct, it has its offset *and* size printed.  No
>>>>> matter if the field is an int, another struct, or an union.
>>>>>
>>>>> If a field is inside an union, it has only its size printed.
>>>>>
>>>>> In the case above, it makes sense to have the offsets printed for the
>>>>> fields inside the two structs (inside the union), because there might be
>>>>> holes to report (well, one can argue that it doesn't matter if there are
>>>>> holes or not in this case, because if the other struct is bigger then
>>>>> the union size will stay the same).  However, it doesn't make sense to
>>>>> print the offsets for the two structs themselves, because they are
>>>>> members of the union.
>>>>>
>>>>> I hope it makes more sense now.
>>>>
>>>> But why do we need the special case?  Does it help anything?
>>>> So far, it seems it only added confusion.
>>>
>>> What do you mean by "special case"?
>>
>> Special case:
>>
>>   "If a field is inside a union, it has only its size printed; otherwise
>>    print the offset and size." 
>>
>> No special case:
>>
>>   "Always print the offset and size."
> 
> I don't like the expression "special case" because it diminishes the
> difference that exist between structs and unions.  

I don't follow, but OK...

> It is not like I went
> out of my way to treat this difference and made the code complex; it is
> also not like the output is extremely complex with it.

The point isn't about the implementation complexity, it's about
user expectations.  Simon was seemingly surprised by "an inconsistency"
(i.e., a case is not consistent with the others; i.e., there's
special/different case), so I think it's valid to discuss a bit and maybe
double check the rationale and see if we can save users from being confused too.
If after chatting a bit we come to the conclusion skipping the offsets
makes sense, than that's fine.  No harm done.

>>> I don't consider this a
>>> special case; I consider it to be the natural thing to do, because
>>> offsets don't make much sense in unions.
>>
>> Of course they do.  You can do 'offsetof(foo_union, foo_field)' just
>> fine, for example.  Saying that the offsets happen to be the same
>> is not the same as saying that the offsets don't exist.
> 
> I don't remember saying offsets don't exist in unions.  What I said is
> that in this specific case they don't matter/make much sense to be
> printed.

Guess we're discussing semantics, which is kind of pointless...
"Don't make sense" to me is like talking about what's the 
"weight of a mile", which is truly meaningless.  Stating that
all union members live at offset 0 is not meaningless, because
that's exactly how you define a union!

>> So from that angle, I see value in not printing the offsets
>> of union members.
> 
> Since it's still not clear whether the offsets should be printed or not
> in this case, and I am not a global maintainer, I adjusted the code to
> print them and will post the patch as a reply to the v4 e-mail.  This
> way you can decide which version is best.

Fun, just when I agreed with not printing the offsets... :-P  :-)

Thanks,
Pedro Alves