[cfe-commits] [PATCH] Instantiate dependent attributes in template code.

[Richard Smith]

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 <delesley at google.com>
> wrote:
>
>> Cool.  Thanks for the clarification; I'll refactor as needed.
>>
>>
>>  -DeLesley
>>
>>
>> On Tue, Jan 17, 2012 at 10:42 AM, Richard Smith <richard at metafoo.co.uk>
>> 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 <richard at metafoo.co.uk>
>>>> 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
>>>>>> <richard at metafoo.co.uk>
>>>>>> 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).