[PATCH] D50526: Model type attributes as regular Attrs

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1510
+  let Spellings = [Keyword<"__unsafe_unretained">];
+  let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> 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?
This isn't actually the primary representation of `__unsafe_unretained`, this is an internal placeholder for "the user wrote `__unsafe_unretained` but we're not in ARC mode so it's meaningless (hence "inert")". So I don't think this is where we should attach the documentation (the right place is the `ObjCOwnership` attribute, which is also currently `Undocumented`, sadly). I'll at least add a comment to the .td file to explain that.


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

The pointee type is deduced as `const` anyway, so the `const` doesn't give any extra type safety. So the only benefit would be to the reader, and I don't think a reader of this code would care whether `*AT` is mutable, so the `const` seems like a distraction to me (and hence the explicit `const` is a minor harm to readability rather than a minor improvement). I can see how a reader trained to think that absence-of-`const` implies that mutation is intended would find the explicit `const` clearer, but (for better or probably worse) that's not the style we generally use in Clang (for instance, I didn't mark the `AK` parameter as `const`, but there is no implication that I intend to modify it).

That said, I don't feel strongly about this, and I certainly have no objection to making the change here. If we generally want to move to a style where `auto` is always accompanied by an explicit `const` when `const` would be deduced (in much the same way that we already expect that `auto` be accompanied by an explicit `*` when a pointer type would be deduced), I'd be OK with that -- but we should discuss that somewhere more visible than this review thread and include it in our general coding guidelines.


================
Comment at: lib/AST/Type.cpp:3215
   }
-  llvm_unreachable("bad attributed type kind");
 }
----------------
aaron.ballman wrote:
> Probably need to keep this to prevent MSVC from barking about not all control paths returning a value.
There's a `default:` case now, so that shouldn't be a problem. (Removing the exhaustive list is intended to be temporary; I'd like to move to generating this with TableGen sooner rather than later.)


================
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.
----------------
aaron.ballman wrote:
> "We use the keep the attributes" isn't grammatical. Should it be, "We keep the attributes in creation order" instead?
Hah, oops, yes.


================
Comment at: lib/Sema/SemaType.cpp:181
+    // line up properly.
+    using TypeAttr = std::pair<const AttributedType*, const Attr*>;
+    SmallVector<TypeAttr, 8> TypeAttrs;
----------------
aaron.ballman wrote:
> 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
Good point. I went with `TypeAttrPair` here, and `AttrsForTypes` in the vector.


================
Comment at: lib/Sema/SemaType.cpp:3884
 
+template<typename AttrT>
+static AttrT *createSimpleAttr(ASTContext &Ctx, ParsedAttr &Attr) {
----------------
aaron.ballman wrote:
> Did clang-format do this? It seems like it's not formatted how I'd expect.
How would you expect it? clang-format puts a space after the `template` keyword, unfortunately, and IIRC can't be configured to not do so. Though as a consequence, it looks like the space is now more common in clang code by a 2:1 ratio despite being "clearly wrong" ;-(


================
Comment at: lib/Sema/SemaType.cpp:6360
+  }
+}
+
----------------
aaron.ballman wrote:
> MSVC may complain about not all control paths returning a value here.
I'm confident that this pattern is fine; we use it all over the place. MSVC knows that control flow cannot leave an `llvm_unreachable(...)`.


================
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());
----------------
aaron.ballman wrote:
> Identifiers don't meet coding style rules.
Fixed. (This was pre-existing, but I changed everything else in the function.)


================
Comment at: lib/Serialization/ASTWriter.cpp:4485-4486
+  push_back(Attrs.size());
+  for (const auto *A : Attrs)
+    AddAttr(A);
 }
----------------
aaron.ballman wrote:
> `llvm::for_each(Attrs, AddAttr);` ?
`AddAttr` is a non-static member function, so that doesn't work; the closest we can get would be something like `llvm::for_each(Attrs, [&](const Attr *A) { AddAttr(A); });` -- but that seems unnecessarily circumlocutory to me.


https://reviews.llvm.org/D50526