[PATCH] D67368: [NFCI]Create CommonAttributeInfo Type as base type of *Attr and ParsedAttr.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for doing all this work -- in general, this is looking great and really cleans things up!



================
Comment at: clang/include/clang/Basic/Attr.td:2266
   let Spellings = [CXX11<"", "maybe_unused", 201603>, GCC<"unused">,
-                   C2x<"", "maybe_unused">];
+                   C2x<"", "maybe_unused">, Pragma<"", "unused">];
   let Subjects = SubjectList<[Var, ObjCIvar, Type, Enum, EnumConstant, Label,
----------------
I'm surprised to see this as part of an NFC refactoring. Hopefully it will become more obvious later. :-D


================
Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:66-69
+  unsigned AttrKind : 16;
+  /// Corresponds to the Syntax enum.
+  unsigned SyntaxUsed : 3;
+  unsigned SpellingIndex : 4;
----------------
Do you want to do `= 0` here to give them in-class initializers? (well, `SpellingIndex` should probably be `SpellingNotCalculated`).


================
Comment at: clang/include/clang/Lex/Preprocessor.h:373
 
-  /// The source location of the currently-active
+  /// The source location and identifier of the currently-active
   /// \#pragma clang arc_cf_code_audited begin.
----------------
Can you correct the order in the comment to be the same as the order in the pair?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:298-300
+          AttributeCommonInfo({}, AttributeCommonInfo::AT_Aligned,
+                              AttributeCommonInfo::AS_GNU,
+                              AlignedAttr::GNU_aligned)));
----------------
I'm surprised we'd give a parse syntax to an implicit attribute. Wouldn't we rather note that these have no associated syntax or spelling?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:892
     = (VisibilityAttr::VisibilityType) rawType;
+
   SourceLocation loc = Stack->back().second;
----------------
Spurious change?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8962-8965
+    SourceLocation Loc = D.getDeclSpec().getNoreturnSpecLoc();
+    AttributeCommonInfo Info(SourceRange{Loc}, AttributeCommonInfo::AT_NoReturn,
+                             AttributeCommonInfo::AS_Keyword);
+    NewFD->addAttr(::new (Context) C11NoReturnAttr(Context, Info));
----------------
It would be nice if we didn't have to repeat information, if possible. The previous version of this didn't need to specify `AT_NoReturn` because the `C11NoReturnAttr` was sufficient to glean that information. It would be awesome if we could find a way to make this more succinct. Perhaps `ConcreteAttr::Create(Context, Range, Syntax, Spelling = DefaultValue)` or something along those lines?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13880
           fmt = "NSString";
-        FD->addAttr(FormatAttr::CreateImplicit(Context,
-                                               &Context.Idents.get(fmt),
-                                               FormatIdx+1,
-                                               HasVAListArg ? 0 : FormatIdx+2,
-                                               FD->getLocation()));
+        FD->addAttr(FormatAttr::CreateImplicit(
+            Context, &Context.Idents.get(fmt), FormatIdx + 1,
----------------
Spurious formatting change? Same for the next three changes.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3349
+    AttributeCommonInfo Info{SourceRange(VS.getOverrideLoc()),
+                             AttributeCommonInfo::UnknownAttribute,
+                             AttributeCommonInfo::AS_Keyword, 0};
----------------
Can we use something other than `UnknownAttribute` here? That implies we don't know the spelling of the attribute name, which seems wrong.


================
Comment at: clang/lib/Sema/SemaDeclObjC.cpp:4464-4465
     // merge the attribute into implementation.
-    method->addAttr(
-      ObjCRequiresSuperAttr::CreateImplicit(S.Context,
-                                            method->getLocation()));
+    method->addAttr(ObjCRequiresSuperAttr::CreateImplicit(
+        S.Context, method->getLocation()));
   }
----------------
Spurious formatting change.


================
Comment at: clang/lib/Sema/SemaObjCProperty.cpp:2414-2415
     if (property->hasAttr<NSReturnsNotRetainedAttr>())
-      GetterMethod->addAttr(NSReturnsNotRetainedAttr::CreateImplicit(Context,
-                                                                     Loc));
+      GetterMethod->addAttr(
+          NSReturnsNotRetainedAttr::CreateImplicit(Context, Loc));
 
----------------
I'll stop pointing out the formatting changes, but you should probably drop these from the patch.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:50
   FnScope->setHasFallthroughStmt();
-  return ::new (S.Context) auto(Attr);
+  return ::new (S.Context) FallThroughAttr(S.Context, A);
 }
----------------
Thank you for this change. I have no idea how the original code made it through a code review.


================
Comment at: clang/lib/Sema/SemaType.cpp:3942
   Attr.setUsedAsTypeAttr();
-  return ::new (Ctx)
-      AttrT(Attr.getRange(), Ctx, Attr.getAttributeSpellingListIndex());
+  return ::new (Ctx) AttrT(Ctx, Attr);
 }
----------------
Since you're touching this code already... can you rename `Attr` to not conflict with a type name? :-D


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:56
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
----------------
Is this still needed?


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:4526
+
+  Record.AddIdentifierRef(A->getAttrName());
+  Record.AddIdentifierRef(A->getScopeName());
----------------
Do we also need bump an AST binary format version number for this sort of change?


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

https://reviews.llvm.org/D67368