[PATCH] D31338: Move ParsedAttrInfos into a registry and point to one in ParsedAttr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:45
+struct ParsedAttrInfo {
+  /// Corresponds to the Kind enum
+  unsigned AttrKind : 16;
----------------
Please add a full stop to the end of all the comments (here and elsewhere).


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:63
+  unsigned IsSupportedByPragmaAttribute : 1;
+  // The syntaxes supported by this attribute and how they're spelt
+  struct Spelling {
----------------
spelt -> spelled


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:615
   AttributeCommonInfo::Kind getKind() const { return getParsedKind(); }
+  ParsedAttrInfo &getInfo() const { return *Info; }
 };
----------------
I think this should return a `const ParsedAttrInfo&` so that callers don't try to mutate it.


================
Comment at: clang/lib/Basic/Attributes.cpp:101-103
+  for (ParsedAttrInfoRegistry::iterator it = ParsedAttrInfoRegistry::begin(),
+                                        ie = ParsedAttrInfoRegistry::end();
+       it != ie; ++it) {
----------------
Range-based for loop? Also, `it` and `ie` don't meet the usual naming conventions.

One thing I'm not keen on with this is the performance hit. We spent a decent amount of effort making this lookup fast and it's now a linear search through all of the attributes and requires memory allocations and deallocations to perform the search.


================
Comment at: clang/lib/Basic/Attributes.cpp:113
+  // If we failed to find a match then the attribute is unknown
+  return std::make_unique<ParsedAttrInfo>();
+}
----------------
This seems surprising because it makes it harder to determine whether you have a valid result from this function or not. I think this should return a null `unique_ptr`.


================
Comment at: clang/lib/Basic/Attributes.cpp:117
+std::unique_ptr<ParsedAttrInfo>
+ParsedAttrInfo::get(AttributeCommonInfo::Kind K) {
+  // Search for a ParsedAttrInfo whose kind matches
----------------
Same concerns in this function as above.


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

https://reviews.llvm.org/D31338