[PATCH] D63954: Add lifetime categories attributes
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 23 08:14:59 PDT 2019
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4167
+
+The attribute ``[[gsl::Owner(T)]]`` applies to structs and classes that own an
+object of type ``T``:
----------------
Do either of these attributes make sense on a union type? If so, might be worth mentioning unions here and below. If not, would it be worth diagnosing on a union?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2516
"%0 and %1 attributes are not compatible">;
+def err_attribute_invalid_argument : Error<
+ "%0 is an invalid argument to attribute %1">;
----------------
Can you combine this one with `err_attribute_argument_vec_type_hint`?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:385
+ TheParsedType = T.get();
+ break; // Multiple type arguments are not implemented.
+ } else if (Tok.is(tok::identifier) &&
----------------
Can you add a FIXME prefix to the comment?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4550
+ S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument)
+ << "A reference type" << AL;
+ return;
----------------
This should say `a reference type`, but I prefer seeing this done in a `%select{}` within the diagnostic rather than manually adding string literals. This will also simplify the logic down to:
```
unsigned SelectIdx = ~0U;
if (ParamType->isVoidType())
SelectIdx = 0;
else if (ParamType->isReferenceType())
SelectIdx = 1;
else if (ParamType->isArrayType())
SelectIdx = 2;
if (SelectIdx != ~0U) {
S.Diag(...) << SelectIdx << AL;
return;
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63954/new/
https://reviews.llvm.org/D63954
More information about the cfe-commits
mailing list