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

[Delesley Hutchins]

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 <richard at metafoo.co.uk> 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 <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).
>



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315