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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
