[cfe-commits] [PATCH][MS][Review request] - Microsoft C anonymous struct, second try

[John McCall]

On Nov 8, 2010, at 9:35 PM, Francois Pichet wrote:

> On Fri, Nov 5, 2010 at 3:21 AM, John McCall <rjmccall at apple.com> wrote:
>> On Nov 4, 2010, at 12:24 PM, Francois Pichet wrote:
>>> Anybody planning to review that?
>> 
>> Sorry, I should have reviewed this earlier.
>> 
>> This approach seems pretty fragile — which is not your fault, it's just that our history with anonymous structs/unions has been that they've been subject to a long succession of bugs due to poor AST representation, and I foresee the same applying here, particularly since the feature is specific to a platform that most contributors aren't testing with.  So I'd like to counter-propose an idea, and if you aren't certain how to implement this, let me know and we can try to figure something out.
>> 
>> The commonality between this feature and anonymous structs/unions is that a name is added to a struct which actually refers to a member of a subobject of that struct.  It seems to me that the best "unifying" approach would be to add some sort of NestedFieldDecl (IndirectFieldDecl?  anyway, a subclass of ValueDecl) which consists of a chain of two or more declarations.  When injecting fields for a GNU/C++ anonymous structs and union, for each field we would inject a NestedFieldDecl whose path would terminate in the appropriate field;  similarly, for MS anonymous structs, for each field of that struct we would inject a NestedFieldDecl leading to it.  All the code in member access, etc., which checks for a member of an anonymous struct or union would instead check for a NestedFieldDecl and expand it out into the same sequence of accesses that we expand into today.
>> 
>> For top-level anonymous structs or unions, the first declaration in a NestedFieldDecl's chain would be a VarDecl;  otherwise it would be a FieldDecl.  In all cases, all the remaining decls would be FieldDecls.
>> 
>> I think this new representation would naturally support both of these features in a way that would be straightforward to correctly support.  Let me know what you think and, if you approve, whether you think you're ready to implement this sort of deep change to the AST.
>> 
>> John.
> 
> Hi I am currently implementing this.

Great!

> 1. I prefer IndirectFieldDecl to NestedFieldDecl. is that ok?

Sure, implementor's choice.  But do consider whether "IndirectFieldDecl" is an equally good name for top-level anonymous struct members.

> 2. I think IndirectFieldDecl should derive from FieldDecl instead of
> ValueDecl. Are you ok with this?
> The reason is that currently a name lookup on an anonymous field
> results in a FieldDecl. After my patch it will result in an
> IndirectFieldDecl. Some places in the code expect a FieldDecl after a
> name lookup (ie: BuildMemberInitializer). Having IndirectFieldDecl
> derives from FieldDecl seems logical and let an IndirectFieldDecl to
> impersonate a FieldDecl which is what we want.

I disagree.  The top-level anonymous struct/union ("union { int x; };") case is important but doesn't really make sense as a FieldDecl.  Put another way, I think it's quite unlikely that an arbitrary piece of code expecting a FieldDecl is going to do the right thing if it instead gets a IndirectFieldDecl.  Giving them disjoint implementation types will help flush out bugs like that a lot faster.

For example, I bet every client which examines a member initializer either already has a special case for anonymous struct/union members or has a latent bug about it. :)

But feel free to modify predicates like isCXXInstanceMember() and extract out new ones where necessary.

John.