[cfe-commits] [PATCH] Fix tag decls/enum constants in function prototypes

[Chandler Carruth]

A couple of high-level comments:

- Clang doesn't use date-based test cases, and I don't want to start. The
date is completely irrelevant to the test case, they should be named based
on the semantics they're checking.

- You currently check both code generation and semantic analysis in the
same test, they need to be split into two tests, one for Sema and one for
CodeGen. You might be able to eliminate the CodeGen test completely if you
can find a way to observe the value of the enumerator entirely within the
frontend. This is easy with C++ by using a static assert, not sure off hand
about C.

- You shouldn't ever add a warning to the warning-flags.c test. The goal of
that test is to ensure we never add a new warning without a warning flag
associated with it. You should provide a flag to control the new warning
you're introducing.

- You need to add visiting logic for all of these decls to the various
visitors I suspect. We have several, I'd have to go looking to enumerate
all of them. Off the top of my head I would check:
  - RecursiveASTVisitor descends into these decls.
  - The various printing and dumping visitors catch them
  - The CFG builder handles them (not sure how much it really has to care)
  - Maybe update libclang? Not likely important for the first pass.


Specific comments on the patch:

Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h (revision 148299)
+++ include/clang/Sema/Sema.h (working copy)
@@ -886,6 +886,16 @@
   // Symbol table / Decl tracking callbacks: SemaDecl.cpp.
   //

+private:
+  /// List of decls defined in a function prototype. This contains
EnumConstants
+  /// that incorrectly end up in translation unit scope because there is no
+  /// function to pin them on. ActOnFunctionDeclarator reads this list and
patches
+  /// them into the FunctionDecl.
+  std::vector<NamedDecl*> DeclsInPrototypeScope;
+  bool InFunctionDeclarator;
+
+public:
+

Why make these private? That's not common in Sema because of the large
number of file-local static helper functions which need to reach into the
innards of Sema. These in particular seem like reasonable things to query
from anywhere that has the Sema object.

Secondly, why place these here? Sema is a just a grab bag of crap, but you
could probably profitably stash these nearer to the function that is most
likely to use them, or with some other Sema state-management fields.

Index: include/clang/AST/Decl.h
===================================================================
--- include/clang/AST/Decl.h (revision 148299)
+++ include/clang/AST/Decl.h (working copy)
@@ -1388,6 +1388,13 @@
   /// no formals.
   ParmVarDecl **ParamInfo;

+  /// DeclsInPrototypeScope - new[]'d array of pointers to NamedDecls for
+  /// decls defined in the function prototype that are not parameters. E.g.
+  /// 'AA' in 'void f(enum {AA} x) {}'. This is null if a prototype or if
there
+  /// are no such decls.
+  NamedDecl **DeclsInPrototypeScope;
+  unsigned NumDeclsInPrototypeScope;
+

I assume that this is a raw new[]'d array to allow us to allocate it using
the ASTContext BumpPtrAllocator? we really need to thread allocators into
the LLVM ADTs. :: sigh ::

If this is the case, I would suggest making the member a ArrayRef<NamedDecl
*> or MutableArrayRef<NamedDecl *>.

@@ -1753,6 +1761,20 @@
     setParams(getASTContext(), NewParamInfo);
   }

+  unsigned getNumDeclsInPrototypeScope() const { return
NumDeclsInPrototypeScope; }
+
+  const NamedDecl *getDeclInPrototypeScope(unsigned i) const {
+    assert(i < NumDeclsInPrototypeScope);
+    return DeclsInPrototypeScope[i];
+  }
+  NamedDecl *getDeclInPrototypeScope(unsigned i) {
+    assert(i < NumDeclsInPrototypeScope);
+    return DeclsInPrototypeScope[i];
+  }

Why not just provide access via an ArrayRef? That would seem a cleaner and
simpler interface.

+  void setDeclsInPrototypeScope(llvm::ArrayRef<NamedDecl *> NewDecls) {
+    setDeclsInPrototypeScope(getASTContext(), NewDecls);
+  }
+

Do we really need this wrapper?

Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp (revision 148299)
+++ lib/Sema/SemaDecl.cpp (working copy)
@@ -1218,6 +1218,11 @@
   }
 }

+void Sema::ActOnStartFunctionDeclarator() {
+  DeclsInPrototypeScope.clear();

I would clear this once we move them to the new function decl below.

@@ -7075,6 +7090,14 @@
     }
   }

+  // If we had any enum constants defined in the function prototype,
+  // introduce them to the function scope.
+  for (unsigned i = 0, N = FD->getNumDeclsInPrototypeScope(); i < N; ++i) {
+    NamedDecl *D = FD->getDeclInPrototypeScope(i);

The convention for for loops is more 'i' and 'e', but if you make this an
array ref it becomes super easy just to get begin and end pointers and
treat it like an iterator.

+    if (FnBodyScope)

Isn't this loop-invariant? Seems like it should guard the whole loop.

@@ -8028,7 +8051,21 @@
                   !isa<CXXRecordDecl>(Def) ||

 cast<CXXRecordDecl>(Def)->getTemplateSpecializationKind()
                                                ==
TSK_ExplicitSpecialization) {
-                Diag(NameLoc, diag::err_redefinition) << Name;
+                // A redeclaration in function prototype scope in C isn't
+                // visible elsewhere, so merely issue a warning.
+                bool FoundPrototypeScope = false;
+                Scope *S2 = S;
+                while (S2) {
+                  if (S2->isFunctionPrototypeScope()) {
+                    FoundPrototypeScope = true;
+                    break;
+                  }
+                  S2 = S2->getParent();
+                }

This loop should be hoisted into a helper function, likely on Scope.

+                if (FoundPrototypeScope && !getLangOptions().CPlusPlus)

And then make sure you check CPlusPlus first so we don't do the recursive
walk up the scopes when we don't need to.

+                  Diag(NameLoc, diag::warn_redefinition_in_param_list) <<
Name;
+                else
+                  Diag(NameLoc, diag::err_redefinition) << Name;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120126/cf9c169b/attachment.html>