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