llvm dev - Mar 2014 - [LLVMdev] [RFC] The coding standard for "struct" should be relaxed or removed

On Mar 1, 2014, at 7:15 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> On Sat, Mar 1, 2014 at 5:59 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> The current guidelines [1] on the use of the struct keyword are too
> restrictive and apparently ignored.  They limit the use of struct to
> PODs, citing broken compilers.
> 
> The guidelines are out-of-date and should be relaxed.  Here’s why:
> 
>  1. Our updated list of supported compilers should all deal correctly
>     with non-POD structs.
> 
>  2. A quick grep of the source suggests that no one paid attention
>     anyway.
> 
> I’ve attached a patch that removes the guideline entirely (matching
> the apparent current practice), but does anyone feel a new explicit
> guideline is in order?
> 
> We should at least remove it, the rule is nonsense as you point out.
This rule was specifically about brokenness with some version of MSVC.  That
version (I have no idea which, or if it still does that) mangled classes and
structs differently.  If this isn't the case for the currently supported
version of MSVC, we should definitely remove this guideline.

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140301/a850de0a/attachment.html>

On Sat, Mar 1, 2014 at 10:33 PM, Chris Lattner <clattner at apple.com>
wrote:
>
> This rule was specifically about brokenness with some version of MSVC.
>  That version (I have no idea which, or if it still does that) mangled
> classes and structs differently.  If this isn't the case for the
currently
> supported version of MSVC, we should definitely remove this guideline.
>
MSVC still mangles the tag into the type, making the manglings for class
and struct differ.  We even have code for this in MicrosoftMangle.cpp.  The
consequence is that you have to use the right tag when you forward declare
something, which we can spot with -Wmismatched-tags.  I don't think this is
a very big deal, and we can probably remove the rule.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140301/35f8024e/attachment.html>

On 2014 Mar 1, at 22:33, Chris Lattner <clattner at apple.com> wrote:
> On Mar 1, 2014, at 7:15 PM, Chandler Carruth <chandlerc at
google.com> wrote:
>> On Sat, Mar 1, 2014 at 5:59 PM, Duncan P. N. Exon Smith <dexonsmith
at apple.com> wrote:
>> The current guidelines [1] on the use of the struct keyword are too
>> restrictive and apparently ignored.  They limit the use of struct to
>> PODs, citing broken compilers.
>> 
>> The guidelines are out-of-date and should be relaxed.  Here’s why:
>> 
>>  1. Our updated list of supported compilers should all deal correctly
>>     with non-POD structs.
>> 
>>  2. A quick grep of the source suggests that no one paid attention
>>     anyway.
>> 
>> I’ve attached a patch that removes the guideline entirely (matching
>> the apparent current practice), but does anyone feel a new explicit
>> guideline is in order?
>> 
>> We should at least remove it, the rule is nonsense as you point out.
> 
> This rule was specifically about brokenness with some version of MSVC. 
That version (I have no idea which, or if it still does that) mangled classes
and structs differently.  If this isn't the case for the currently supported
version of MSVC, we should definitely remove this guideline.
> 
> -Chris
According to <http://www.agner.org/optimize/calling_conventions.pdf>
(see table 9), MSVC still mangles classes and structs differently.

Nevertheless, the rule is not being followed.  E.g., df_ext_iterator,
APInt::ms, ilist_traits<SparseBitVectorElement<ElementSize>>, and
CaptureTracker are four quite different examples that run afoul (none
of these is POD).  It’s easy to get this wrong; there are many more
examples.

I’m not even sure the rule is helpful.  Even POD types can be
alternately declared “class” or “struct”, changing their mangling in
MSVC.

I’ve attached a patch that clarifies the mangling problem and
describes the actual practice in the codebase.  How does this look?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clarify-struct-guidelines.patch
Type: application/octet-stream
Size: 1425 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140301/7eb902c2/attachment.obj>

Works for me,

-Chris
> On Mar 1, 2014, at 9:53 PM, "Duncan P. N. Exon Smith"
<dexonsmith at apple.com> wrote:
> 
> 
>> On 2014 Mar 1, at 22:33, Chris Lattner <clattner at apple.com>
wrote:
>> 
>>> On Mar 1, 2014, at 7:15 PM, Chandler Carruth <chandlerc at
google.com> wrote:
>>> On Sat, Mar 1, 2014 at 5:59 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>>> The current guidelines [1] on the use of the struct keyword are too
>>> restrictive and apparently ignored.  They limit the use of struct
to
>>> PODs, citing broken compilers.
>>> 
>>> The guidelines are out-of-date and should be relaxed.  Here’s why:
>>> 
>>> 1. Our updated list of supported compilers should all deal
correctly
>>>    with non-POD structs.
>>> 
>>> 2. A quick grep of the source suggests that no one paid attention
>>>    anyway.
>>> 
>>> I’ve attached a patch that removes the guideline entirely (matching
>>> the apparent current practice), but does anyone feel a new explicit
>>> guideline is in order?
>>> 
>>> We should at least remove it, the rule is nonsense as you point
out.
>> 
>> This rule was specifically about brokenness with some version of MSVC. 
That version (I have no idea which, or if it still does that) mangled classes
and structs differently.  If this isn't the case for the currently supported
version of MSVC, we should definitely remove this guideline.
>> 
>> -Chris
> 
> According to <http://www.agner.org/optimize/calling_conventions.pdf>
> (see table 9), MSVC still mangles classes and structs differently.
> 
> Nevertheless, the rule is not being followed.  E.g., df_ext_iterator,
> APInt::ms, ilist_traits<SparseBitVectorElement<ElementSize>>,
and
> CaptureTracker are four quite different examples that run afoul (none
> of these is POD).  It’s easy to get this wrong; there are many more
> examples.
> 
> I’m not even sure the rule is helpful.  Even POD types can be
> alternately declared “class” or “struct”, changing their mangling in
> MSVC.
> 
> I’ve attached a patch that clarifies the mangling problem and
> describes the actual practice in the codebase.  How does this look?
> 
> <clarify-struct-guidelines.patch>