[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2771
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
----------------
This subject should be `Struct` and same below.


================
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">;
----------------
mgehre wrote:
> aaron.ballman wrote:
> > Can you combine this one with `err_attribute_argument_vec_type_hint`?
> I'm not sure: vec_type_hint reads `"invalid attribute argument %0 - expecting a vector or vectorizable scalar type"` and here `""%0 is an invalid argument to attribute %1"`, i.e. one is positive ("expecting ...") and the other is negative ("%0 is an invalid argument").
> 
> I don't know how to describe "not void, not reference, not array type" in terms of "expecting ...", and I think that we should keep "expecting a vector or vectorizable scalar type" on the  VecTypeHint attribute diagnostic.
I had figured it would be something like `%select{'void'|a reference type|an array type|a non-vector or non-vectorizable scalar type}0 is an invalid argument to attribute %1` but I am not strongly opposed if you want to keep the diagnostics separate.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2933
+  "|non-K&R-style functions"
+  "|classes}1">,
   InGroup<IgnoredAttributes>;
----------------
You can drop this change when the `Subjects` field is fixed above.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:1037
   ExpectedFunctionWithProtoType,
+  ExpectedClass,
 };
----------------
Same


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4538-4542
+  if (!RD || RD->isUnion()) {
+    S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
+        << AL << ExpectedClass;
+    return;
+  }
----------------
You can drop this bit when the Subjects field is fixed above.


================
Comment at: clang/test/Misc/pragma-attribute-supported-attributes-list.test:119
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
----------------
These will also wind up needing to be updated when switching the `Subjects` field.


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp:2
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
----------------
This file tests part of parsing and part of Sema somewhat interchangeably. I'm not strictly opposed to it being that way, but it would be a bit cleaner to have the parsing tests in the Parser directory and the rest in SemaCXX (that also reduces the confusion from the file name).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63954/new/

https://reviews.llvm.org/D63954