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

[John McCall]

On Nov 16, 2010, at 11:01 PM, Francois Pichet wrote:
> After this patch is approved, I'll submit another patch for the
> ms-anonymous struct using the same infrastructure.
> After that I think some redesign could be done in SemInit.cpp
> regarding anonymous handling. I didn't do it in this patch because I
> think this patch is already large enough.

Seems reasonable.

@@ -180,6 +181,10 @@
 
   /// \brief Determine whether this declaration is a C++ class member.
   bool isCXXClassMember() const {
+    // IndirectField are always class member.
+    if (isa<IndirectFieldDecl>(this))
+      return true;
+

This isn't correct.  Also, you shouldn't need to modify this method.

+class IndirectFieldDecl : public ValueDecl {
+  llvm::SmallVector<NamedDecl*, 2> Chaining;

You can't use SmallVector in an AST node;  AST nodes are never destructed.
It needs to be an array allocated by the ASTContext.  Feel free to make
ASTReader a friend.

Whatever actually creates the storage should assert that the length is >= 2.
 
+protected:
+  IndirectFieldDecl(DeclContext *DC, SourceLocation L,
+                    DeclarationName N, QualType T)
+    : ValueDecl(IndirectField, DC, L, N, T) {}
+
+public:
+  static IndirectFieldDecl *Create(ASTContext &C, DeclContext *DC,
+                                   SourceLocation L, IdentifierInfo *Id,
+                                   QualType T);
+  
+  void SetChaining(const llvm::SmallVector<NamedDecl*, 2>& C) { Chaining = C; }

We don't need a setter for this.

+  const llvm::SmallVector<NamedDecl*, 2>& GetChaining() { return Chaining; }

Iterators, please.  chain_begin() and chain_end().

+  FieldDecl *getAnonField() const {
+    assert(Chaining.size() > 0);
+    return dyn_cast<FieldDecl>(Chaining.back());
+  }

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.

       /// \brief A FieldDecl record.
       DECL_FIELD,
+      /// \brief A IndirectFieldDecl record.
+      DECL_INDIRECTFIELD,
       /// \brief A VarDecl record.
       DECL_VAR,

Please put this at the end of the enum;  we've tried to keep this numbering
consistent, just in case it ever matters.

+  llvm::SmallVector<NamedDecl *, 2> Chaining;
+  Chaining.reserve(NumParams);
+  for (unsigned I = 0; I != NumParams; ++I)
+    Chaining.push_back(cast<NamedDecl>(Importer.Import(D->GetChaining()[I])));

Importer.Import can fail;  please test for null returns.

+++ lib/Sema/SemaDecl.cpp	(working copy)

+        
+        llvm::SmallVector<NamedDecl*, 2> ChainingCopy = Chaining;
+        ChainingCopy.push_back(*F);

You can just push *F onto Chaining here and then pop it after
IndirectFieldDecl::Create returns.  Similarly for the recursive case.

@@ -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.

+++ lib/Sema/SemaDeclCXX.cpp	(working copy)
@@ -1067,11 +1067,16 @@
     FieldDecl *Member = 0;
     DeclContext::lookup_result Result
       = ClassDecl->lookup(MemberOrBase);
-    if (Result.first != Result.second)
+    if (Result.first != Result.second) {
       Member = dyn_cast<FieldDecl>(*Result.first);
+    
+	  // Handle anonymous union case.
+	  if (!Member)
+      if (IndirectFieldDecl* IndirectField
+          = dyn_cast<IndirectFieldDecl>(*Result.first))
+        Member = IndirectField->getAnonField();
+  }
 
-    // FIXME: Handle members of an anonymous union.
-

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

@@ -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)) {
    ...
  }

If you find yourself making this check a lot, feel free to add a method to
Decl like isAnyFieldDecl().

-        break;
+	      break;

Watch the tabs.

+++ 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:

+    if (i == 0 && dyn_cast<VarDecl>(Field->GetChaining()[i]))
+      BaseObject = cast<VarDecl>(Field->GetChaining()[i]);
+    else if (FieldDecl *F = dyn_cast<FieldDecl>(Field->GetChaining()[i]))
+      Path.insert(Path.begin(), F);    
+    else
+      llvm_unreachable(0);

Please walk the chain backwards instead of repeatedly inserting at the
front of a vector.  Also, this code will be much plainer if you pull the check
for an initial var out of the loop.

+    
+    if (dyn_cast<IndirectFieldDecl>(D))
+      return IMA_AnonymousMember;
+

isa<>, please, but note:

-
-      // If this is a member of an anonymous record, move out to the
-      // innermost non-anonymous struct or union.  If there isn't one,
-      // that's a special case.
-      while (R->isAnonymousStructOrUnion()) {
-        R = dyn_cast<CXXRecordDecl>(R->getParent());
-        if (!R) return IMA_AnonymousMember;
-      }

This logic only returns IMA_AnonymousMember for non-class-members.
But we don't actually end up in this method at all in that case anymore,
so IMA_AnonymousMember can just disappear.

Instead of trying to redirect non-class IndirectFieldDecls into
BuildPossibleImplicitMemberExpr, it should be fine for just have BuildDeclarationNameExpr
(the override taking a single declaration) test for IndirectFieldDecl
and call BuildAnonymousStructUnionMemberReference.

John.