Hi Chandler,
Thanks for the thorough review. My comments inline. Cheers, James From: Chandler Carruth [mailto:[email protected]] Sent: 26 January 2012 10:52 To: James Molloy Cc: [email protected]; [email protected]; [email protected] Subject: Re: [cfe-commits] [PATCH] Fix tag decls/enum constants in function prototypes 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. No problem. This is a convention in LLVM’s test dirs and I hadn’t clocked that Clang didn’t have the same convention. WILLFIX. - 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. Sure – I will go about doing this. - 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. Separation of concerns is good. Yes, I’m adding a new warning that has no flag, but it’s essentially a clone of another warning with one verb different that currently has no flag. Do you suggest I add the flag to cover both the old and new warnings in the same patch? I can do it as a followup and I’d prefer that if possible. - 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. Thanks for catching this – I didn’t think it would be required. I’ll ensure everything acts as it should, although I’m not sure how much effect this will have. The decls already existed previously, just were in the wrong scope. 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. I’m sure I had a reason but it evades me. 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. The reason is simple – that place is the start of the functions that are used in SemaDecl.cpp (see the comment in the first line of context in the diff). I placed the variables that are used in SemaDecl.cpp right above the functions that use them. 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 *>. It’s raw new’d for the reason you state. I didn’t use arrayref because the existing code for ParmVarDecl (see diff context) also doesn’t use ArrayRef. I copied its interface completely, assuming (for better or worse) that it was designed that way for a reason. @@ -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. See above, copying existing coding style. While ArrayRef is better, I didn’t want to mix coding styles in one class unless required. I can go through and change if you want? + void setDeclsInPrototypeScope(llvm::ArrayRef<NamedDecl *> NewDecls) { + setDeclsInPrototypeScope(getASTContext(), NewDecls); + } + Do we really need this wrapper? Again, above. 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. Sure. @@ -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. Quite possibly. WILLFIX @@ -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. Will do. + 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. Will do. + Diag(NameLoc, diag::warn_redefinition_in_param_list) << Name; + else + Diag(NameLoc, diag::err_redefinition) << Name;
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
