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

[John Brawn via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/ParsedAttr.cpp:143
+  // otherwise return a default ParsedAttrInfo.
+  const ParsedAttrInfo *Info = AttrInfoMap[A.getKind()];
+  if (Info)
----------------
erichkeane wrote:
> I don't think you can do this.  The only way to get Info as nullptr is to end up off the end of the array, which is UB.
> 
> Instead of the map lookup, this should check A.getKind against the range.
AttrInfoMap is declared to have size ParsedAttr::UnknownAttribute+1, so when the kind is too large we get an element that wasn't explicitly initialized so will be null.

Doing it by kind makes more sense though, so I'll do that.


================
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) {
----------------
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.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3637
+    SS << "ParsedAttrInfo" << I->first;
+    SS << " parsedAttrInfo" << I->first << "Instance;\n\n";
   }
----------------
erichkeane wrote:
> Should the instance be static? Perhaps a static variable in the class itself?  
Making it static makes sense, I'll do that.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3641
 
-  OS << "static const ParsedAttrInfo AttrInfoMap[ParsedAttr::UnknownAttribute "
+  OS << "static const ParsedAttrInfo *AttrInfoMap[ParsedAttr::UnknownAttribute "
         "+ 1] = {\n";
----------------
erichkeane wrote:
> Is there a good reason this doesn't return references?
You can't have an array of references (C++11 dcl.ref paragraph 5).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31337