[cfe-commits] [PATCH][Review request] - Anonymous union/struct redesign

[John McCall]

On Nov 20, 2010, at 7:10 AM, Francois Pichet wrote:
> On Wed, Nov 17, 2010 at 3:19 AM, John McCall <rjmccall at apple.com> wrote:
>> 
>> This can be cast<FieldDecl>;  the final member will always be a FieldDecl.
>> 
>> Please add an accessor to get the base declaration, i.e. *chain_begin().  It
>> might also help to have asserting accessors to grab the first member as a
>> FieldDecl or VarDecl.
>> 
> 
> Accessor to get the base declaration for what? I didn't feel the need of it.

I mean the first declaration, i.e. the thing that makes this equivalent to either a variable or a field.  But if you haven't seen much use for this, then let's keep the interface simple for now.

>> @@ -1920,6 +1934,8 @@
>>         }
>>       } else if (isa<AccessSpecDecl>(*Mem)) {
>>         // Any access specifier is fine.
>> +      } else if (isa<IndirectFieldDecl>(*Mem)) {
>> +        // Any access specifier is fine.
>> 
>> Fix the comment, please.
> 
> I actually removed that code. It is not necessary.

Great!

>> I think this would be cleaner if we built the member initializer with an
>> IndirectFieldDecl*, but we can fix that in a later patch.
> 
> Yes later patch, this require changes in SemaInit.cpp

Okay.

>> @@ -2599,15 +2604,21 @@
>>     //   In addition, if class T has a user-declared constructor (12.1), every
>>     //   non-static data member of class T shall have a name different from T.
>>     for (DeclContext::lookup_result R = Record->lookup(Record->getDeclName());
>> -         R.first != R.second; ++R.first)
>> +         R.first != R.second; ++R.first) {
>>       if (FieldDecl *Field = dyn_cast<FieldDecl>(*R.first)) {
>> -        if (Record->hasUserDeclaredConstructor() ||
>> -            !Field->getDeclContext()->Equals(Record)) {
>> +        if (Record->hasUserDeclaredConstructor()) {
>> +         Diag(Field->getLocation(), diag::err_member_name_of_class)
>> +            << Field->getDeclName();
>> +          break;
>> +        }
>> +      }
>> +      else if (IndirectFieldDecl *Field =
>> +                    dyn_cast<IndirectFieldDecl>(*R.first)) {
>>         Diag(Field->getLocation(), diag::err_member_name_of_class)
>>           << Field->getDeclName();
>> 
>> This can be
>>  NamedDecl *D = *R.first;
>>  if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D)) {
>>    ...
>>  }
> 
> What is needed is:
>      NamedDecl *D = *R.first;
>      if ((isa<FieldDecl>(D) && Record->hasUserDeclaredConstructor()) ||
>          isa<IndirectFieldDecl>(D)) {

[class.union]p5:
  The names of the members of an anonymous union shall be distinct from
  the names of any other entity in the scope in which the anonymous union
  is declared.

So this applies equally to indirect fields.

>> +++ lib/Sema/SemaExpr.cpp       (working copy)
>> 
>> +VarDecl *Sema::BuildAnonymousStructUnionMemberPath(IndirectFieldDecl *Field,
>>                                    llvm::SmallVectorImpl<FieldDecl *> &Path) {
>> 
>> I'm not convinced this method needs to exist anymore, but if it does:
> 
> Why not? this function is called at 2 places. That code shall not be duplicated.
> <anonymous2.patch>

This function doesn't do useful work anymore.  It used to be useful because the
path wasn't explicit in the AST, but now its callers can just iterate over the path
in the IndirectFieldDecl instead of copying that out into a vector and iterating
over that.  In fact, all of its callers should already know by context whether the
base declaration is a field or a variable.

John.