Thanks! Chandler, can you take a look at the changes to the build files? http://codereview.appspot.com/5556059/
-DeLesley On Thu, Jan 19, 2012 at 1:37 PM, Richard Smith <[email protected]> wrote: > Hi DeLesley, > > I have a couple of minor comments: > > --- a/utils/TableGen/ClangAttrEmitter.cpp > +++ b/utils/TableGen/ClangAttrEmitter.cpp > @@ -853,3 +932,62 @@ void ClangAttrLateParsedListEmitter::run(raw_ostream > &OS) { > } > } > } > + > + > +void ClangAttrTemplateInstantiateEmitter::run(raw_ostream &OS) { > + OS << "// This file is generated by TableGen. Do not edit.\n\n"; > + > + std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr"); > + > + OS << "Attr* instantiateTemplateAttribute(const Attr *At, ASTContext &C, " > + << "Sema &S,\n" > + << " const MultiLevelTemplateArgumentList &TemplateArgs) {\n" > + << " switch (At->getKind()) {\n" > + << " default: {\n" > + << " assert(0 && \"Unknown attribute!\");\n" > + << " return 0;\n" > + << " }\n"; > > Can you move this out of the switch, and change the assert to an > llvm_unreachable? We're moving to that style for switches over covered > enumerations, and, while this is generated code, it should still ideally > conform. > > + > + for (std::vector<Record*>::iterator I = Attrs.begin(), E = Attrs.end(); > + I != E; ++I) { > + Record &R = **I; > + > + OS << " case attr::" << R.getName() << ": {\n"; > + OS << " const " << R.getName() << "Attr *A = reinterpret_cast<const > " > > Use cast<...> rather than reinterpret_cast<...>, please. > > + << R.getName() << "Attr*>(At);\n"; > + bool TDependent = R.getValueAsBit("TemplateDependent"); > + > + if (!TDependent) { > + OS << " return A->clone(C);\n"; > + OS << " }\n"; > + continue; > + } > + > + std::vector<Record*> ArgRecords = R.getValueAsListOfDefs("Args"); > + std::vector<Argument*> Args; > + std::vector<Argument*>::iterator ai, ae; > + Args.reserve(ArgRecords.size()); > + > + for (std::vector<Record*>::iterator ri = ArgRecords.begin(), > + re = ArgRecords.end(); > + ri != re; ++ri) { > + Record &ArgRecord = **ri; > + Argument *Arg = createArgument(ArgRecord, R.getName()); > + assert(Arg); > + Args.push_back(Arg); > + } > + ae = Args.end(); > + > + for (ai = Args.begin(); ai != ae; ++ai) { > + (*ai)->writeTemplateInstantiation(OS); > + } > + OS << " return new (C) " << R.getName() << "Attr(A->getLocation(), > C"; > + for (ai = Args.begin(); ai != ae; ++ai) { > + OS << ", "; > + (*ai)->writeTemplateInstantiationArgs(OS); > + } > + OS << ");\n }\n"; > + } > + OS << " } // end switch\n}\n\n"; > +} > + > > Functionally (subject to the above minor issues), this looks good to me. > However, I'd like someone more familiar with the build systems to take a look > at those changes and make sure they look OK. > > - Richard > > On Thu, January 19, 2012 19:27, Delesley Hutchins wrote: >> New patch is enclosed. Since template instantiation is no longer in a >> method, I can't duplicate the code for clone as the default case, due to >> private member variables. Instead, I mark attributes as explicitly >> dependent, >> and simply call clone for non-dependent attributes. >> >> http://codereview.appspot.com/5556059/ >> >> >> -DeLesley >> >> >> On Tue, Jan 17, 2012 at 10:45 AM, Delesley Hutchins <[email protected]> >> wrote: >> >>> Cool. Thanks for the clarification; I'll refactor as needed. >>> >>> >>> -DeLesley >>> >>> >>> On Tue, Jan 17, 2012 at 10:42 AM, Richard Smith <[email protected]> >>> wrote: >>> >>>> On Tue, January 17, 2012 16:07, Delesley Hutchins wrote: >>>> >>>>> I'm still a bit unclear -- I want to make sure I understand which >>>>> dependencies cause a layering violation, so I can avoid this mistake in >>>>> the future. There's still no dependency in the call graph, because >>>>> instantiateFromTemplate is only called from within Sema. There is a >>>>> dependency in the object file though, because Attr must be linked >>>>> against Sema to instantiate the virtual method table. Is the goal to be >>>>> able to package the AST classes into a separate library that does not >>>>> need to be linked against Sema? >>>> >>>> Yes. Sorry for not stating this explicitly! >>>> >>>> >>>>> On Fri, Jan 13, 2012 at 5:13 PM, Richard Smith <[email protected]> >>>>> wrote: >>>>> >>>>>> On Fri, January 13, 2012 23:03, Delesley Hutchins wrote: >>>>>> >>>>>>> The instantiateFromTemplate method is only called from within Sema. >>>>>>> Is the layering violation introduced by the forward declaration of >>>>>>> class Sema within Attr.h? >>>>>> >>>>>> It's introduced by the call of Sema::SubstExpr from >>>>>> instantiateFromTemplate. >>>>>> >>>>>>> On Fri, Jan 13, 2012 at 2:33 PM, Richard Smith >>>>>>> <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Delesley, >>>>>>>> >>>>>>>> >>>>>>>> Apologies for the huge delay in getting this patch reviewed! >>>>>>>> >>>>>>>> >>>>>>>> On Tue, December 13, 2011 23:08, Delesley Hutchins wrote: >>>>>>>> >>>>>>>>> This patch modifies Sema::InstantiateAttrs so that attributes >>>>>>>>> in template code are properly instantiated; the previous >>>>>>>>> behavior was to clone them. The main motivation for the patch >>>>>>>>> are thread safety attributes, which make extensive use of >>>>>>>>> expressions. >>>>>>>>> >>>>>>>>> A new method named instantiateFromTemplate is now generated for >>>>>>>>> all attributes. The behavior of this method is identical to >>>>>>>>> clone() for all arguments except ExprArgument and >>>>>>>>> VariadicExprArgument; expression >>>>>>>>> arguments are instantiated rather than cloned. >>>>>>>>> >>>>>>>> >>>>>>>> This patch introduces a layering violation: AST code is not >>>>>>>> permitted to use Sema. In order to resolve this, I suggest you >>>>>>>> instead modify TableGen to >>>>>>>> synthesize some Sema code which performs attribute instantiation >>>>>>>> (using >>>>>>>> a switch over the attribute kind). > -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
