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