<div class="gmail_quote">A couple of high-level comments:</div><div class="gmail_quote"><br></div><div class="gmail_quote">- 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.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">- 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.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">- 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.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">- 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:</div>
<div class="gmail_quote"> - RecursiveASTVisitor descends into these decls.</div><div class="gmail_quote"> - The various printing and dumping visitors catch them</div><div class="gmail_quote"> - The CFG builder handles them (not sure how much it really has to care)</div>
<div class="gmail_quote"> - Maybe update libclang? Not likely important for the first pass.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Specific comments on the patch:</div><div class="gmail_quote"><br></div><div class="gmail_quote"><div class="gmail_quote">Index: include/clang/Sema/Sema.h</div>
<div class="gmail_quote">
===================================================================</div><div class="gmail_quote">--- include/clang/Sema/Sema.h<span style="white-space:pre-wrap"> </span>(revision 148299)</div><div class="gmail_quote">
+++ include/clang/Sema/Sema.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div class="gmail_quote">@@ -886,6 +886,16 @@</div><div class="gmail_quote"> // Symbol table / Decl tracking callbacks: SemaDecl.cpp.</div>
<div class="gmail_quote"> //</div><div class="gmail_quote"> </div><div class="gmail_quote">+private:</div><div class="gmail_quote">+ /// List of decls defined in a function prototype. This contains EnumConstants</div>
<div class="gmail_quote">
+ /// that incorrectly end up in translation unit scope because there is no</div><div class="gmail_quote">+ /// function to pin them on. ActOnFunctionDeclarator reads this list and patches</div><div class="gmail_quote">
+ /// them into the FunctionDecl.</div><div class="gmail_quote">+ std::vector<NamedDecl*> DeclsInPrototypeScope;</div><div class="gmail_quote">+ bool InFunctionDeclarator;</div><div class="gmail_quote">+</div><div class="gmail_quote">
+public:</div><div class="gmail_quote">+</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div><div>Index: include/clang/AST/Decl.h</div><div>===================================================================</div><div>--- include/clang/AST/Decl.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 148299)</div>
<div>+++ include/clang/AST/Decl.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -1388,6 +1388,13 @@</div><div> /// no formals.</div><div> ParmVarDecl **ParamInfo;</div><div> </div>
<div>+ /// DeclsInPrototypeScope - new[]'d array of pointers to NamedDecls for</div><div>+ /// decls defined in the function prototype that are not parameters. E.g.</div><div>+ /// 'AA' in 'void f(enum {AA} x) {}'. This is null if a prototype or if there</div>
<div>+ /// are no such decls.</div><div>+ NamedDecl **DeclsInPrototypeScope;</div><div>+ unsigned NumDeclsInPrototypeScope;</div><div>+</div></div><div><br></div><div>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 ::</div>
<div><br></div><div>If this is the case, I would suggest making the member a ArrayRef<NamedDecl *> or MutableArrayRef<NamedDecl *>.</div><div><br></div><div><div>@@ -1753,6 +1761,20 @@</div><div> setParams(getASTContext(), NewParamInfo);</div>
<div> }</div><div> </div><div>+ unsigned getNumDeclsInPrototypeScope() const { return NumDeclsInPrototypeScope; }</div><div>+</div><div>+ const NamedDecl *getDeclInPrototypeScope(unsigned i) const {</div><div>+ assert(i < NumDeclsInPrototypeScope);</div>
<div>+ return DeclsInPrototypeScope[i];</div><div>+ }</div><div>+ NamedDecl *getDeclInPrototypeScope(unsigned i) {</div><div>+ assert(i < NumDeclsInPrototypeScope);</div><div>+ return DeclsInPrototypeScope[i];</div>
<div>+ }</div><div><br></div><div>Why not just provide access via an ArrayRef? That would seem a cleaner and simpler interface.</div><div><br></div><div>+ void setDeclsInPrototypeScope(llvm::ArrayRef<NamedDecl *> NewDecls) {</div>
<div>+ setDeclsInPrototypeScope(getASTContext(), NewDecls);</div><div>+ }</div><div>+</div></div><div><br></div><div>Do we really need this wrapper?</div><div><br></div><div><div>Index: lib/Sema/SemaDecl.cpp</div><div>
===================================================================</div><div>--- lib/Sema/SemaDecl.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 148299)</div><div>+++ lib/Sema/SemaDecl.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -1218,6 +1218,11 @@</div><div> }</div><div> }</div><div> </div><div>+void Sema::ActOnStartFunctionDeclarator() {</div><div>+ DeclsInPrototypeScope.clear();</div><div><br></div><div>I would clear this once we move them to the new function decl below.</div>
</div><div><br></div><div><div>@@ -7075,6 +7090,14 @@</div><div> }</div><div> }</div><div> </div><div>+ // If we had any enum constants defined in the function prototype,</div><div>+ // introduce them to the function scope.</div>
<div>+ for (unsigned i = 0, N = FD->getNumDeclsInPrototypeScope(); i < N; ++i) {</div><div>+ NamedDecl *D = FD->getDeclInPrototypeScope(i);</div><div><br></div><div>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.</div>
<div><br></div><div>+ if (FnBodyScope)</div><div><br></div><div>Isn't this loop-invariant? Seems like it should guard the whole loop.</div><div><br></div></div><div><div>@@ -8028,7 +8051,21 @@</div><div> !isa<CXXRecordDecl>(Def) ||</div>
<div> cast<CXXRecordDecl>(Def)->getTemplateSpecializationKind() </div><div> == TSK_ExplicitSpecialization) {</div><div>- Diag(NameLoc, diag::err_redefinition) << Name;</div>
<div>+ // A redeclaration in function prototype scope in C isn't</div><div>+ // visible elsewhere, so merely issue a warning.</div><div>+ bool FoundPrototypeScope = false;</div>
<div>+ Scope *S2 = S;</div><div>+ while (S2) {</div><div>+ if (S2->isFunctionPrototypeScope()) {</div><div>+ FoundPrototypeScope = true;</div><div>+ break;</div>
<div>+ }</div><div>+ S2 = S2->getParent();</div><div>+ }</div><div><br></div><div>This loop should be hoisted into a helper function, likely on Scope.</div><div><br></div>
<div>+ if (FoundPrototypeScope && !getLangOptions().CPlusPlus)</div><div><br></div><div>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.</div>
<div><br></div><div>+ Diag(NameLoc, diag::warn_redefinition_in_param_list) << Name;</div><div>+ else</div><div>+ Diag(NameLoc, diag::err_redefinition) << Name;</div>
</div><div><br></div></div>