[PATCH] D50526: Model type attributes as regular Attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this -- the approach you've taken looks good, and I mostly have little nits here and there. I think this is a great refactoring overall though -- much needed!



================
Comment at: include/clang/AST/Type.h:1856
+
+  /// Was this type written with the special inert-in-MRC __unsafe_unretained
+  /// qualifier?
----------------
Typo: MRC should be ARC (typo was present in the original code).


================
Comment at: include/clang/Basic/Attr.td:1510
+  let Spellings = [Keyword<"__unsafe_unretained">];
+  let Documentation = [Undocumented];
+}
----------------
I don't suppose you can throw in some quick docs for this keyword? Or is this not really user-facing? If it's not user-facing, perhaps it should have no spellings and only ever be created implicitly?


================
Comment at: lib/AST/Type.cpp:1624
+  const Type *Cur = this;
+  while (auto *AT = Cur->getAs<AttributedType>()) {
+    if (AT->getAttrKind() == AK)
----------------
`const auto *`


================
Comment at: lib/AST/Type.cpp:3215
   }
-  llvm_unreachable("bad attributed type kind");
 }
----------------
Probably need to keep this to prevent MSVC from barking about not all control paths returning a value.


================
Comment at: lib/AST/Type.cpp:3653
+Optional<NullabilityKind>
+Type::getNullability(const ASTContext &context) const {
   QualType type(this, 0);
----------------
Update the identifiers for coding standards since you're basically rewriting the function?


================
Comment at: lib/AST/Type.cpp:3655
   QualType type(this, 0);
-  do {
+  while (auto *AT = type->getAs<AttributedType>()) {
     // Check whether this is an attributed type with nullability
----------------
`const auto *`


================
Comment at: lib/AST/TypeLoc.cpp:408
   if (auto attributedLoc = getAs<AttributedTypeLoc>()) {
-    if (attributedLoc.getAttrKind() == AttributedType::attr_nullable ||
-        attributedLoc.getAttrKind() == AttributedType::attr_nonnull ||
-        attributedLoc.getAttrKind() == AttributedType::attr_null_unspecified)
-      return attributedLoc.getAttrNameLoc();
+    auto *A = attributedLoc.getAttr();
+    if (A && (isa<TypeNullableAttr>(A) || isa<TypeNonNullAttr>(A) ||
----------------
Might as well go with `const Attr *` here, the `auto` gets us nothing.


================
Comment at: lib/Sema/SemaDecl.cpp:6024
       // by applying it to the function type.
-      if (ATL.getAttrKind() == AttributedType::attr_lifetimebound) {
+      if (auto *A = ATL.getAttrAs<LifetimeBoundAttr>()) {
         const auto *MD = dyn_cast<CXXMethodDecl>(FD);
----------------
`const auto *`


================
Comment at: lib/Sema/SemaType.cpp:178-180
+    // their TypeLocs makes it hard to correctly assign these. We use the
+    // keep the attributes in creation order as an attempt to make them
+    // line up properly.
----------------
"We use the keep the attributes" isn't grammatical. Should it be, "We keep the attributes in creation order" instead?


================
Comment at: lib/Sema/SemaType.cpp:181
+    // line up properly.
+    using TypeAttr = std::pair<const AttributedType*, const Attr*>;
+    SmallVector<TypeAttr, 8> TypeAttrs;
----------------
It's unfortunate to use the name `TypeAttr` here -- unqualified uses of the type name, like below, makes it look like it might be the `TypeAttr` from Attr.h


================
Comment at: lib/Sema/SemaType.cpp:3884
 
+template<typename AttrT>
+static AttrT *createSimpleAttr(ASTContext &Ctx, ParsedAttr &Attr) {
----------------
Did clang-format do this? It seems like it's not formatted how I'd expect.


================
Comment at: lib/Sema/SemaType.cpp:6360
+  }
+}
+
----------------
MSVC may complain about not all control paths returning a value here.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2695
+  Attr *New = nullptr;
+  auto Kind = (attr::Kind)(V - 1);
+  SourceRange Range = Record.readSourceRange();
----------------
This could use a comment to explain why the -1 is required. Also, do we have a preference for C-style vs static_cast here?


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2707
 void ASTReader::ReadAttributes(ASTRecordReader &Record, AttrVec &Attrs) {
-  for (unsigned i = 0, e = Record.readInt(); i != e; ++i) {
-    Attr *New = nullptr;
-    auto Kind = (attr::Kind)Record.readInt();
-    SourceRange Range = Record.readSourceRange();
-    ASTContext &Context = getContext();
-
-#include "clang/Serialization/AttrPCHRead.inc"
-
-    assert(New && "Unable to decode attribute?");
-    Attrs.push_back(New);
-  }
+  for (unsigned i = 0, e = Record.readInt(); i != e; ++i)
+    Attrs.push_back(Record.readAttr());
----------------
Identifiers don't meet coding style rules.


================
Comment at: lib/Serialization/ASTWriter.cpp:4485-4486
+  push_back(Attrs.size());
+  for (const auto *A : Attrs)
+    AddAttr(A);
 }
----------------
`llvm::for_each(Attrs, AddAttr);` ?


Repository:
  rC Clang

https://reviews.llvm.org/D50526