[PATCH] D50526: Model type attributes as regular Attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! Thank you for this fantastic work!



================
Comment at: include/clang/Basic/Attr.td:1510
+  let Spellings = [Keyword<"__unsafe_unretained">];
+  let Documentation = [Undocumented];
+}
----------------
rsmith wrote:
> 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.
Ah, thank you for the explanation; I agree.


================
Comment at: lib/AST/Type.cpp:1624
+  const Type *Cur = this;
+  while (auto *AT = Cur->getAs<AttributedType>()) {
+    if (AT->getAttrKind() == AK)
----------------
rsmith wrote:
> 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.
> 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.

Yeah, I actually thought this already was part of the coding guidelines, truth be told. But I went and looked again and realized we only talk about `*` and `&` being deduced, not qualifiers. I guess my preference has always been to explicitly spell out pertinent information about the type beyond the name, such as whether it's `const`, whether it's a pointer, etc. Basically, things that aren't immediately obvious from the context.

I prefer being explicit about spelling out the `const` here because it makes it obvious that the type is not intended to be mutated, but I only really care about it in cases where the type is deduced to a pointer or reference (and so mutating operations might be hidden behind a function call that looks harmless).

Perhaps this is worth starting a coding guideline discussion over?


================
Comment at: lib/Sema/SemaType.cpp:3884
 
+template<typename AttrT>
+static AttrT *createSimpleAttr(ASTContext &Ctx, ParsedAttr &Attr) {
----------------
rsmith wrote:
> 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" ;-(
I was expecting to see a space after `template` as I thought that was the most common form of it in the code base. :-D


================
Comment at: lib/Sema/SemaType.cpp:6360
+  }
+}
+
----------------
rsmith wrote:
> 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(...)`.
Yeah, I think I was mis-remembering a pattern that caused warnings in MSVC here.


================
Comment at: lib/Serialization/ASTWriter.cpp:4485-4486
+  push_back(Attrs.size());
+  for (const auto *A : Attrs)
+    AddAttr(A);
 }
----------------
rsmith wrote:
> 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.
Yeah, that's a bridge too far. The current form is preferable.


https://reviews.llvm.org/D50526