[PATCH] D31337: Use virtual functions in ParsedAttrInfo instead of function pointers

[John Brawn via Phabricator via cfe-commits]

john.brawn marked 2 inline comments as done.
john.brawn added inline comments.


================
Comment at: clang/lib/Sema/ParsedAttr.cpp:141
 
+static ParsedAttrInfo DefaultParsedAttrInfo;
 static const ParsedAttrInfo &getInfo(const ParsedAttr &A) {
----------------
aaron.ballman wrote:
> Might as well lower this variable into the function -- no real need for it to have file scope.
Sure.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3400
 
-static std::string GenerateAppertainsTo(const Record &Attr, raw_ostream &OS) {
+static void GenerateAppertainsTo(const Record &Attr, raw_ostream &SS,
+                                 raw_ostream &OS) {
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > john.brawn wrote:
> > > erichkeane wrote:
> > > > Why does this take SS AND OS.  What is the difference here?  Does this actually use OS anymore?
> > > It's because of GenerateCustomAppertainsTo.  That generates a function that expects to be at file scope (because emitAttributeMatchRules re-uses the same function), but at the time GenerateAppertainsTo is called SS is in the middle of the ParsedAttrInfo. Previously both the function that GenerateCustomAppertainsTo generates and that this file generates would be at file scope, so both were written to OS.
> > Thanks!
> I think the stream parameter names should be a bit more descriptive here (and perhaps some comments on the function would be a good idea as well). Perhaps `FileScopeStream` and `LexicalScopeStream`?
Actually, looking at this again there's no reason we can't generate the CustomApperatinsTo functions in a loop at the start of EmitClangAttrParsedAttrImpl. That way everything can just go to a single output stream and we don't have this problem. I'll rearrange things to do that instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D31337/new/

https://reviews.llvm.org/D31337