[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I will include your latest comments into D64448 



Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: cfe/trunk/include/clang/Basic/AttrDocs.td:4252
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code

"and is currently ignored"

even better:

"and is ignored"

It does not matter for the user that we might change it in future. We might 
change *anything* in future, and yet we don't hedge everywhere.



Comment at: cfe/trunk/include/clang/Basic/AttrDocs.td:4278
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code

ditto



Comment at: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp:34
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};

Technically correct, but not very helpful for the user.

It would be better if the message was tailored for the attribute, for example,

owner types can't own an object of type 'void'


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Thank you very much for dedicating your time for the review. It improved the 
code a lot!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367040: Add lifetime categories attributes (authored by 
mgehre, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63954?vs=211600&id=211792#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Parse/ParseDecl.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/AST/ast-dump-attr.cpp
  cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
  cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
  cfe/trunk/test/SemaOpenCL/invalid-kernel-attrs.cl
  cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2519,6 +2519,9 @@
   "'NSObject' attribute is for pointer types only">;
 def err_attributes_are_not_compatible : Error<
   "%0 and %1 attributes are not compatible">;
+def err_attribute_invalid_argument : Error<
+  "%select{'void'|a reference type|an array type|a non-vector or "
+  "non-vectorizable scalar type}0 is an invalid argument to attribute %1">;
 def err_attribute_wrong_number_arguments : Error<
   "%0 attribute %plural{0:takes no arguments|1:takes one argument|"
   ":requires exactly %1 arguments}1">;
@@ -2567,8 +2570,6 @@
 def err_init_priority_object_attr : Error<
   "can only use 'init_priority' attribute on file-scope definitions "
   "of objects of class type">;
-def err_attribute_argument_vec_type_hint : Error<
-  "invalid attribute argument %0 - expecting a vector or vectorizable scalar type">;
 def err_attribute_argument_out_of_bounds : Error<
   "%0 attribute parameter %1 is out of bounds">;
 def err_attribute_only_once_per_parameter : Error<
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -4230,3 +4230,71 @@
 the initializer on host side.
   }];
 }
+
+def LifetimeOwnerDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+.. Note:: This attribute is experimental and its effect on analysis is subject to change in
+  a future version of clang.
+
+The attribute ``[[gsl::Owner(T)]]`` applies to structs and classes that own an
+object of type ``T``:
+
+.. code-block:: c++
+
+  class [[gsl::Owner(int)]] IntOwner {
+  private:
+int value;
+  public:
+int *getInt() { return &value; }
+  };
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code
+generation.
+
+See Pointer_ for an example.
+}];
+}
+
+def LifetimePointerDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+.. Note:: This attribute is experimental and its effect on analysis is subject to change in
+  a future version of clang.
+
+The attribute ``[[gsl::Pointer(T)]]`` applies to structs and classes that behave
+like pointers to an object of type ``T``:
+
+.. code-block:: c++
+
+  class [[gsl::Pointer(int)]] IntPointer {
+  private:
+int *valuePointer;
+  public:
+int *getInt() { return &valuePointer; }
+  };
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code
+generation.
+
+Example:
+When constructing an instance of a class annotated like this (a Pointer) from
+an instance of a class annotated with ``[[gsl::Owner]]`` (an Owner),
+then the analysis will consider the Pointer to point inside the Owner.
+When the Owner's lifetime ends, it will consider the Pointer to be dangling.
+
+.. code-block:: c++
+
+  int f() {
+IntPointer P;
+if (true) {
+  IntOwner O(7);
+  P = IntPointer(O); // P "points into" O
+} // P is dangling
+return P.get(); // error: Using a dangling Pointer.
+  }
+
+}];
+}
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -2796,6 +2796,20 @@
   let Documentation = [TypeTagForDatatypeDocs];
 }
 
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[Struct]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let Documentation = [LifetimeOwnerDocs];
+}
+
+def Pointer : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Pointer">];
+  let Subjects = SubjectList<[Struct]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let Documentation = [LifetimePointerDocs];
+}

[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a minor formatting nit. Thank you for working on these 
attributes!




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2517
+def err_attribute_invalid_argument : Error<
+  "%select{'void'|a reference type|an array type|a non-vector or 
non-vectorizable scalar type}0 is an invalid argument to attribute %1">;
 def err_attribute_wrong_number_arguments : Error<

80-col limit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 9 inline comments as done.
mgehre added inline comments.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp:2
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;

aaron.ballman wrote:
> 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).
I had split the tests into two files due to a misconception that the ast-dump 
would not work when errors are emitted. It actually does work, and so I 
recombined the test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 211600.
mgehre marked an inline comment as done.
mgehre added a comment.

- Fix all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/test/SemaOpenCL/invalid-kernel-attrs.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -303,6 +303,8 @@
 std::string getIsOmitted() const override {
   if (type == "IdentifierInfo *")
 return "!get" + getUpperName().str() + "()";
+  if (type == "TypeSourceInfo *")
+return "!get" + getUpperName().str() + "Loc()";
   if (type == "ParamIdx")
 return "!get" + getUpperName().str() + "().isValid()";
   return "false";
@@ -336,6 +338,8 @@
<< "  OS << \" \" << SA->get" << getUpperName()
<< "()->getName();\n";
   } else if (type == "TypeSourceInfo *") {
+if (isOptional())
+  OS << "if (SA->get" << getUpperName() << "Loc())";
 OS << "OS << \" \" << SA->get" << getUpperName()
<< "().getAsString();\n";
   } else if (type == "bool") {
Index: clang/test/SemaOpenCL/invalid-kernel-attrs.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-attrs.cl
+++ clang/test/SemaOpenCL/invalid-kernel-attrs.cl
@@ -1,12 +1,12 @@
-// RUN: %clang_cc1 -verify %s 
+// RUN: %clang_cc1 -verify %s
 
 kernel __attribute__((vec_type_hint)) void kernel1() {} //expected-error{{'vec_type_hint' attribute takes one argument}}
 
 kernel __attribute__((vec_type_hint(not_type))) void kernel2() {} //expected-error{{unknown type name 'not_type'}}
 
-kernel __attribute__((vec_type_hint(void))) void kernel3() {} //expected-error{{invalid attribute argument 'void' - expecting a vector or vectorizable scalar type}}
+kernel __attribute__((vec_type_hint(void))) void kernel3() {} //expected-error{{a non-vector or non-vectorizable scalar type is an invalid argument to attribute 'vec_type_hint'}}
 
-kernel __attribute__((vec_type_hint(bool))) void kernel4() {} //expected-error{{invalid attribute argument 'bool' - expecting a vector or vectorizable scalar type}}
+kernel __attribute__((vec_type_hint(bool))) void kernel4() {} //expected-error{{a non-vector or non-vectorizable scalar type is an invalid argument to attribute 'vec_type_hint'}}
 
 kernel __attribute__((vec_type_hint(int))) __attribute__((vec_type_hint(float))) void kernel5() {} //expected-warning{{attribute 'vec_type_hint' is already applied with different parameters}}
 
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,107 @@
+// RUN: %clang_cc1 -verify -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to structs}}
+
+union [[gsl::Owner(int)]] Union{};
+// expected-warning@-1 {{'Owner' attribute only applies to structs}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+// CHECK: CXXRecordDecl {{.*}} BothOwnerPointer
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+// CHECK: CXXRecordDecl {{.*}} AddConflictLater
+// CHECK: PointerAttr {{.*}} int
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-5 {{conf

[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;

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 211370.
mgehre marked 4 inline comments as done.
mgehre added a comment.

- Diagnose unions; improve other diagnostics; fix all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -303,6 +303,8 @@
 std::string getIsOmitted() const override {
   if (type == "IdentifierInfo *")
 return "!get" + getUpperName().str() + "()";
+  if (type == "TypeSourceInfo *")
+return "!get" + getUpperName().str() + "Loc()";
   if (type == "ParamIdx")
 return "!get" + getUpperName().str() + "().isValid()";
   return "false";
@@ -336,6 +338,8 @@
<< "  OS << \" \" << SA->get" << getUpperName()
<< "()->getName();\n";
   } else if (type == "TypeSourceInfo *") {
+if (isOptional())
+  OS << "if (SA->get" << getUpperName() << "Loc())";
 OS << "OS << \" \" << SA->get" << getUpperName()
<< "().getAsString();\n";
   } else if (type == "bool") {
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} OwnerMissingParameter
+// CHECK: OwnerAttr
+
+class [[gsl::Pointer]] PointerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} PointerMissingParameter
+// CHECK: PointerAttr
+
+class [[gsl::Owner()]] OwnerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} OwnerWithEmptyParameterList
+// CHECK: OwnerAttr {{.*}}
+
+class [[gsl::Pointer()]] PointerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} PointerWithEmptyParameterList
+// CHECK: PointerAttr {{.*}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+// CHECK: CXXRecordDecl {{.*}} AnOwner
+// CHECK: OwnerAttr {{.*}} int
+
+struct S;
+class [[gsl::Pointer(S)]] APointer{};
+// CHECK: CXXRecordDecl {{.*}} APointer
+// CHECK: PointerAttr {{.*}} S
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+// CHECK: CXXRecordDecl {{.*}} DuplicateOwner
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+// CHECK: CXXRecordDecl {{.*}} DuplicatePointer
+// CHECK: PointerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+// CHECK: CXXRecordDecl {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater;
+// CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
Index: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflict

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 2 inline comments as done.
mgehre 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``:

aaron.ballman wrote:
> 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?
Good catch! I added diagnostics and a test.



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">;

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 211008.
mgehre marked 6 inline comments as done.
mgehre added a comment.

- Disallow reference and array types as DerefType
- Add example to documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -303,6 +303,8 @@
 std::string getIsOmitted() const override {
   if (type == "IdentifierInfo *")
 return "!get" + getUpperName().str() + "()";
+  if (type == "TypeSourceInfo *")
+return "!get" + getUpperName().str() + "Loc()";
   if (type == "ParamIdx")
 return "!get" + getUpperName().str() + "().isValid()";
   return "false";
@@ -336,6 +338,8 @@
<< "  OS << \" \" << SA->get" << getUpperName()
<< "()->getName();\n";
   } else if (type == "TypeSourceInfo *") {
+if (isOptional())
+  OS << "if (SA->get" << getUpperName() << "Loc())";
 OS << "OS << \" \" << SA->get" << getUpperName()
<< "().getAsString();\n";
   } else if (type == "bool") {
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} OwnerMissingParameter
+// CHECK: OwnerAttr
+
+class [[gsl::Pointer]] PointerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} PointerMissingParameter
+// CHECK: PointerAttr
+
+class [[gsl::Owner()]] OwnerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} OwnerWithEmptyParameterList
+// CHECK: OwnerAttr {{.*}}
+
+class [[gsl::Pointer()]] PointerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} PointerWithEmptyParameterList
+// CHECK: PointerAttr {{.*}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+// CHECK: CXXRecordDecl {{.*}} AnOwner
+// CHECK: OwnerAttr {{.*}} int
+
+struct S;
+class [[gsl::Pointer(S)]] APointer{};
+// CHECK: CXXRecordDecl {{.*}} APointer
+// CHECK: PointerAttr {{.*}} S
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+// CHECK: CXXRecordDecl {{.*}} DuplicateOwner
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+// CHECK: CXXRecordDecl {{.*}} DuplicatePointer
+// CHECK: PointerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+// CHECK: CXXRecordDecl {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater;
+// CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
Index: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I think that I have addressed all open comments.




Comment at: clang/include/clang/Basic/AttrDocs.td:4164
+  let Content = [{
+When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
+that returns cv-qualified ``T&`` or ``T*`` is assumed to return a

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > Slightly better: "In a class annotated with ..., each function that 
> > > returns ... is assumed to ..."
> > > 
> > > However, the "is assumed" part throws me off. There's no assumption here 
> > > -- this is what this attribute means, intentionally, by design, no 
> > > guessing involved.
> > > 
> > > So how about this?
> > > 
> > > The attribute `[[gsl::Owner(T)]]` applies to structs and classes that own 
> > > an object of type `T`:
> > > 
> > > ```
> > > [[gsl::Owner(int)]] class IntOwner {
> > > private:
> > >   int value;
> > > public:
> > >   int *getInt() { return &value; }
> > >   long *getLong();
> > > };
> > > ```
> > > 
> > > In a class annotated like this each member function that returns `T&` or 
> > > `T*` returns a pointer/reference to the data owned by the class instance:
> > > 
> > > ```
> > > IntOwner int_owner;
> > > int *x = int_owner.getInt();
> > > // `x` points to memory owned by `int_owner`.
> > > ```
> > > 
> > > Therefore, that the lifetime of that data ends when ... For example:
> > > 
> > > ```
> > > int *x = IntOwner().getInt();
> > > // `x` points to memory owned by the temporary `IntOwner()`, which has 
> > > been destroyed now.
> > > 
> > > long *y = IntOwner().getLong();
> > > // Can't make any conclusion about validity of `y`.
> > > ```
> > > 
> > What I meant to express by "is assumed to" is that this attribute does not 
> > change what member functions do. E.g. if I had `int* f() { static int i; 
> > return &i; }`, then `f` would not return data owned by the class instance, 
> > even when annotating the class with `[[gsl::Owner(T)]]`. The point is that 
> > when member functions with return type `T*`/`T&` don't return data owned by 
> > the class instance, one should not add the `[[gsl::Owner(T)]]` attribute - 
> > or the analysis will produce wrong results. Here, we would warn if code 
> > uses the returned value of `f` after the class instances has been destroyed 
> > even though it would be safe.
> > 
> > There is a chicken and egg problem here. With this PR, we don't get any 
> > analysis yet, so providing examples of analysis results like "`x` points to 
> > memory owned by the temporary `IntOwner()`, which has been destroyed now." 
> > can be misleading. On the other hand, if we would just document the current 
> > meaning of the attribute, it would be nothing.
> > The implication "member function that returns `T&` or ` T*` returns a 
> > pointer/reference to the data owned by the class instance" is just one part 
> > of being an Owner. Herb's paper
> > uses this in more ways (e.g. output arguments, handling of move semantics, 
> > etc.), which we would implement incrementally together with the respective 
> > documentation update.
> > 
> > So I propose to keep the documentation basic for this PR and update the 
> > documentation with the upcoming PRs that add to the analysis. Would that be 
> > acceptable?
> > I added a note that the attribute is currently experimental and subject to 
> > change.
> > What I meant to express by "is assumed to" is that this attribute does not 
> > change what member functions do.
> 
> I don't think we should be making allowances for code that uses an attribute 
> to promise something and then does some other thing. In that model, there's 
> alignment between what code does and what attribute says.
> 
> And I mean of course the attribute does not change what the code does... that 
> must be obvious but you can mention it explicitly if you want.
> 
> > With this PR, we don't get any analysis yet
> 
> I don't think the analysis must be automatic in order to show what analysis 
> can be done. Attribute documentation needs enough examples to give people a 
> flavor of what the attribute does, and what possible benefits it can bring.
> 
> Documentation for the specific analysis that is actually done should go 
> elsewhere into Clang's documentation (if necessary).
I added an example to the documentation to show how the initial analysis will 
look like to give a feeling for what the attribute does.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << 
AL;
+return;

gribozavr wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > Is `void` really the only problematic type? What about incomplete types, 
> > > for instance?
> > I think incomplete types are fine. The only information we actually need 
> > here is the name the pointee types. E.g. for a `SomeOwner` 
> > which is annotated `Owner(Incomplete)`

[PATCH] D63954: Add lifetime categories attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:4164
+  let Content = [{
+When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
+that returns cv-qualified ``T&`` or ``T*`` is assumed to return a

mgehre wrote:
> gribozavr wrote:
> > Slightly better: "In a class annotated with ..., each function that returns 
> > ... is assumed to ..."
> > 
> > However, the "is assumed" part throws me off. There's no assumption here -- 
> > this is what this attribute means, intentionally, by design, no guessing 
> > involved.
> > 
> > So how about this?
> > 
> > The attribute `[[gsl::Owner(T)]]` applies to structs and classes that own 
> > an object of type `T`:
> > 
> > ```
> > [[gsl::Owner(int)]] class IntOwner {
> > private:
> >   int value;
> > public:
> >   int *getInt() { return &value; }
> >   long *getLong();
> > };
> > ```
> > 
> > In a class annotated like this each member function that returns `T&` or 
> > `T*` returns a pointer/reference to the data owned by the class instance:
> > 
> > ```
> > IntOwner int_owner;
> > int *x = int_owner.getInt();
> > // `x` points to memory owned by `int_owner`.
> > ```
> > 
> > Therefore, that the lifetime of that data ends when ... For example:
> > 
> > ```
> > int *x = IntOwner().getInt();
> > // `x` points to memory owned by the temporary `IntOwner()`, which has been 
> > destroyed now.
> > 
> > long *y = IntOwner().getLong();
> > // Can't make any conclusion about validity of `y`.
> > ```
> > 
> What I meant to express by "is assumed to" is that this attribute does not 
> change what member functions do. E.g. if I had `int* f() { static int i; 
> return &i; }`, then `f` would not return data owned by the class instance, 
> even when annotating the class with `[[gsl::Owner(T)]]`. The point is that 
> when member functions with return type `T*`/`T&` don't return data owned by 
> the class instance, one should not add the `[[gsl::Owner(T)]]` attribute - or 
> the analysis will produce wrong results. Here, we would warn if code uses the 
> returned value of `f` after the class instances has been destroyed even 
> though it would be safe.
> 
> There is a chicken and egg problem here. With this PR, we don't get any 
> analysis yet, so providing examples of analysis results like "`x` points to 
> memory owned by the temporary `IntOwner()`, which has been destroyed now." 
> can be misleading. On the other hand, if we would just document the current 
> meaning of the attribute, it would be nothing.
> The implication "member function that returns `T&` or ` T*` returns a 
> pointer/reference to the data owned by the class instance" is just one part 
> of being an Owner. Herb's paper
> uses this in more ways (e.g. output arguments, handling of move semantics, 
> etc.), which we would implement incrementally together with the respective 
> documentation update.
> 
> So I propose to keep the documentation basic for this PR and update the 
> documentation with the upcoming PRs that add to the analysis. Would that be 
> acceptable?
> I added a note that the attribute is currently experimental and subject to 
> change.
> What I meant to express by "is assumed to" is that this attribute does not 
> change what member functions do.

I don't think we should be making allowances for code that uses an attribute to 
promise something and then does some other thing. In that model, there's 
alignment between what code does and what attribute says.

And I mean of course the attribute does not change what the code does... that 
must be obvious but you can mention it explicitly if you want.

> With this PR, we don't get any analysis yet

I don't think the analysis must be automatic in order to show what analysis can 
be done. Attribute documentation needs enough examples to give people a flavor 
of what the attribute does, and what possible benefits it can bring.

Documentation for the specific analysis that is actually done should go 
elsewhere into Clang's documentation (if necessary).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 209750.
mgehre marked 23 inline comments as done.
mgehre added a comment.

- Don't crash when pretty-printing an Attribute with optional type argument
- Don't crash AST dump when Owner/Pointer has no Type argument
- gsl::Owner/gsl::Pointer: Make DerefType parameter optional
- Unify type and non-type attribute parsing; don't add OwnerAttr/PointerAttr 
twice
- Improve documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -303,6 +303,8 @@
 std::string getIsOmitted() const override {
   if (type == "IdentifierInfo *")
 return "!get" + getUpperName().str() + "()";
+  if (type == "TypeSourceInfo *")
+return "!get" + getUpperName().str() + "Loc()";
   if (type == "ParamIdx")
 return "!get" + getUpperName().str() + "().isValid()";
   return "false";
@@ -336,6 +338,8 @@
<< "  OS << \" \" << SA->get" << getUpperName()
<< "()->getName();\n";
   } else if (type == "TypeSourceInfo *") {
+if (isOptional())
+  OS << "if (SA->get" << getUpperName() << "Loc())";
 OS << "OS << \" \" << SA->get" << getUpperName()
<< "().getAsString();\n";
   } else if (type == "bool") {
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} OwnerMissingParameter
+// CHECK: OwnerAttr
+
+class [[gsl::Pointer]] PointerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} PointerMissingParameter
+// CHECK: PointerAttr
+
+class [[gsl::Owner()]] OwnerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} OwnerWithEmptyParameterList
+// CHECK: OwnerAttr {{.*}}
+
+class [[gsl::Pointer()]] PointerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} PointerWithEmptyParameterList
+// CHECK: PointerAttr {{.*}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+// CHECK: CXXRecordDecl {{.*}} AnOwner
+// CHECK: OwnerAttr {{.*}} int
+
+struct S;
+class [[gsl::Pointer(S)]] APointer{};
+// CHECK: CXXRecordDecl {{.*}} APointer
+// CHECK: PointerAttr {{.*}} S
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+// CHECK: CXXRecordDecl {{.*}} DuplicateOwner
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+// CHECK: CXXRecordDecl {{.*}} DuplicatePointer
+// CHECK: PointerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+// CHECK: CXXRecordDecl {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater;
+// CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
Index: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;

aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > Has Microsoft already added support for this? We had an unfortunate 
> > > > > problem with `gsl::suppress` where we introduced the attribute first 
> > > > > and MSVC introduced it under a different, incompatible syntax. I 
> > > > > would strongly like to avoid repeating that.
> > > > I will double check this. Her Sutter's paper have this spelling and I 
> > > > believe that the Microsoft implementation is following the same 
> > > > spelling. The main difference is that the MSVC version does not have a 
> > > > type argument at this point but they do plan to add it as an optional 
> > > > argument. (They want to infer the pointee type when it is not 
> > > > provided.) The clang community do not want the inference, hence we made 
> > > > the type argument required. Always providing the type argument should 
> > > > be compatible with future versions of the MSVC implementation.
> > > I take this to mean that we're not implementing the same attribute as 
> > > MSVC is, and that worries me.
> > > 
> > > > Always providing the type argument should be compatible with future 
> > > > versions of the MSVC implementation.
> > > 
> > > The problem is that code written for MSVC (where the type argument is not 
> > > required) will not compile with Clang (where the type argument is 
> > > required), if I understand properly.
> > > 
> > > Generally speaking, when we implement an attribute from another vendor 
> > > namespace, we expect the attribute to have the same semantics as whoever 
> > > "owns" that vendor namespace. It's a bit trickier here because `gsl` 
> > > isn't really a vendor so much as a collective, so I don't know who is the 
> > > authority to turn to.
> > > 
> > > On a completely different note: would this attribute also make sense 
> > > within C with a C2x spelling?
> > I see, that is a valid concern. A followup patch makes the type argument 
> > optional so it will be fully compatible with MSVC.  I don't think it makes 
> > sense with C or at least I am not sure what would I annotate using these 
> > attributes in C code as the subjects are classes. I know that it is 
> > possible to simulate classes in C but I am not sure how well the analysis 
> > would work in such cases. 
> > 
> > Are you OK with making the argument optional in a followup patch or do you 
> > want us to cherry-pick that modification into this one?
> > I don't think it makes sense with C or at least I am not sure what would I 
> > annotate using these attributes in C code as the subjects are classes. I 
> > know that it is possible to simulate classes in C but I am not sure how 
> > well the analysis would work in such cases.
> 
> I couldn't think of a reason to expose this in C either, so I think the 
> current spelling is good as-is.
> 
> > Are you OK with making the argument optional in a followup patch or do you 
> > want us to cherry-pick that modification into this one?
> 
> I'd prefer the argument be optional in this patch.
The argument is now optional.



Comment at: clang/include/clang/Basic/AttrDocs.td:4164
+  let Content = [{
+When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
+that returns cv-qualified ``T&`` or ``T*`` is assumed to return a

gribozavr wrote:
> Slightly better: "In a class annotated with ..., each function that returns 
> ... is assumed to ..."
> 
> However, the "is assumed" part throws me off. There's no assumption here -- 
> this is what this attribute means, intentionally, by design, no guessing 
> involved.
> 
> So how about this?
> 
> The attribute `[[gsl::Owner(T)]]` applies to structs and classes that own an 
> object of type `T`:
> 
> ```
> [[gsl::Owner(int)]] class IntOwner {
> private:
>   int value;
> public:
>   int *getInt() { return &value; }
>   long *getLong();
> };
> ```
> 
> In a class annotated like this each member function that returns `T&` or `T*` 
> returns a pointer/reference to the data owned by the class instance:
> 
> ```
> IntOwner int_owner;
> int *x = int_owner.getInt();
> // `x` points to memory owned by `int_owner`.
> ```
> 
> Therefore, that the lifetime of that data ends when ... For example:
> 
> ```
> int *x = IntOwner().getInt();
> // `x` points to memory owned by the temporary `IntOwner()`, which has been 
> destroyed now.
> 
> long *y = IntOwner().getLong();
> // Can't make any conclusion about validity of `y`.
> ```
> 
What I meant to express by "is assumed to" is that this attribute does not 
change what member functions do. E.g. if I had `int* f() { static int i; return 
&i; }`, then `f` would not return data owned by the class instance, even

[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:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;

xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > Has Microsoft already added support for this? We had an unfortunate 
> > > > problem with `gsl::suppress` where we introduced the attribute first 
> > > > and MSVC introduced it under a different, incompatible syntax. I would 
> > > > strongly like to avoid repeating that.
> > > I will double check this. Her Sutter's paper have this spelling and I 
> > > believe that the Microsoft implementation is following the same spelling. 
> > > The main difference is that the MSVC version does not have a type 
> > > argument at this point but they do plan to add it as an optional 
> > > argument. (They want to infer the pointee type when it is not provided.) 
> > > The clang community do not want the inference, hence we made the type 
> > > argument required. Always providing the type argument should be 
> > > compatible with future versions of the MSVC implementation.
> > I take this to mean that we're not implementing the same attribute as MSVC 
> > is, and that worries me.
> > 
> > > Always providing the type argument should be compatible with future 
> > > versions of the MSVC implementation.
> > 
> > The problem is that code written for MSVC (where the type argument is not 
> > required) will not compile with Clang (where the type argument is 
> > required), if I understand properly.
> > 
> > Generally speaking, when we implement an attribute from another vendor 
> > namespace, we expect the attribute to have the same semantics as whoever 
> > "owns" that vendor namespace. It's a bit trickier here because `gsl` isn't 
> > really a vendor so much as a collective, so I don't know who is the 
> > authority to turn to.
> > 
> > On a completely different note: would this attribute also make sense within 
> > C with a C2x spelling?
> I see, that is a valid concern. A followup patch makes the type argument 
> optional so it will be fully compatible with MSVC.  I don't think it makes 
> sense with C or at least I am not sure what would I annotate using these 
> attributes in C code as the subjects are classes. I know that it is possible 
> to simulate classes in C but I am not sure how well the analysis would work 
> in such cases. 
> 
> Are you OK with making the argument optional in a followup patch or do you 
> want us to cherry-pick that modification into this one?
> I don't think it makes sense with C or at least I am not sure what would I 
> annotate using these attributes in C code as the subjects are classes. I know 
> that it is possible to simulate classes in C but I am not sure how well the 
> analysis would work in such cases.

I couldn't think of a reason to expose this in C either, so I think the current 
spelling is good as-is.

> Are you OK with making the argument optional in a followup patch or do you 
> want us to cherry-pick that modification into this one?

I'd prefer the argument be optional in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;

aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > Has Microsoft already added support for this? We had an unfortunate 
> > > problem with `gsl::suppress` where we introduced the attribute first and 
> > > MSVC introduced it under a different, incompatible syntax. I would 
> > > strongly like to avoid repeating that.
> > I will double check this. Her Sutter's paper have this spelling and I 
> > believe that the Microsoft implementation is following the same spelling. 
> > The main difference is that the MSVC version does not have a type argument 
> > at this point but they do plan to add it as an optional argument. (They 
> > want to infer the pointee type when it is not provided.) The clang 
> > community do not want the inference, hence we made the type argument 
> > required. Always providing the type argument should be compatible with 
> > future versions of the MSVC implementation.
> I take this to mean that we're not implementing the same attribute as MSVC 
> is, and that worries me.
> 
> > Always providing the type argument should be compatible with future 
> > versions of the MSVC implementation.
> 
> The problem is that code written for MSVC (where the type argument is not 
> required) will not compile with Clang (where the type argument is required), 
> if I understand properly.
> 
> Generally speaking, when we implement an attribute from another vendor 
> namespace, we expect the attribute to have the same semantics as whoever 
> "owns" that vendor namespace. It's a bit trickier here because `gsl` isn't 
> really a vendor so much as a collective, so I don't know who is the authority 
> to turn to.
> 
> On a completely different note: would this attribute also make sense within C 
> with a C2x spelling?
I see, that is a valid concern. A followup patch makes the type argument 
optional so it will be fully compatible with MSVC.  I don't think it makes 
sense with C or at least I am not sure what would I annotate using these 
attributes in C code as the subjects are classes. I know that it is possible to 
simulate classes in C but I am not sure how well the analysis would work in 
such cases. 

Are you OK with making the argument optional in a followup patch or do you want 
us to cherry-pick that modification into this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;

xazax.hun wrote:
> aaron.ballman wrote:
> > Has Microsoft already added support for this? We had an unfortunate problem 
> > with `gsl::suppress` where we introduced the attribute first and MSVC 
> > introduced it under a different, incompatible syntax. I would strongly like 
> > to avoid repeating that.
> I will double check this. Her Sutter's paper have this spelling and I believe 
> that the Microsoft implementation is following the same spelling. The main 
> difference is that the MSVC version does not have a type argument at this 
> point but they do plan to add it as an optional argument. (They want to infer 
> the pointee type when it is not provided.) The clang community do not want 
> the inference, hence we made the type argument required. Always providing the 
> type argument should be compatible with future versions of the MSVC 
> implementation.
I take this to mean that we're not implementing the same attribute as MSVC is, 
and that worries me.

> Always providing the type argument should be compatible with future versions 
> of the MSVC implementation.

The problem is that code written for MSVC (where the type argument is not 
required) will not compile with Clang (where the type argument is required), if 
I understand properly.

Generally speaking, when we implement an attribute from another vendor 
namespace, we expect the attribute to have the same semantics as whoever "owns" 
that vendor namespace. It's a bit trickier here because `gsl` isn't really a 
vendor so much as a collective, so I don't know who is the authority to turn to.

On a completely different note: would this attribute also make sense within C 
with a C2x spelling?



Comment at: clang/lib/Parse/ParseDecl.cpp:335-341
+  if (attributeIsTypeArgAttr(*AttrName)) {
+ParseAttributeWithTypeArg(*AttrName, AttrNameLoc, Attrs, EndLoc, ScopeName,
+  ScopeLoc, Syntax);
+// FIXME: when attributeIsTypeArgAttr() is true, assumes that the attribute
+// takes a single parameter.
+return 1;
+  }

I don't like how this short-circuits the remainder of the logic in this 
function. I'd rather see this one-off logic handled in 
`ParseClangAttributeArgs()` if we have to treat it as a one-off. A better 
approach would be to implement type arguments within 
`ParseAttributeArgsCommon()`.



Comment at: clang/lib/Sema/SemaDecl.cpp:2618-2619
+} else if (isa(NewAttribute) || isa(NewAttribute)) 
{
+  // gsl::Owner and gsl::Pointer are allowed to be added to a class after 
it
+  // is defined.
+  ++I;

I'd like to see some tests for this.

I'm a bit worried about whether this will do good things in practice or not. 
I'm concerned about situations like:
```
struct S;

void func(S *s) {
  // Interesting things happen here.
}

struct [[gsl::Pointer]] S {};
```
and whether the "interesting things" will be properly [un]diagnosed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:4164
+  let Content = [{
+When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
+that returns cv-qualified ``T&`` or ``T*`` is assumed to return a

Slightly better: "In a class annotated with ..., each function that returns ... 
is assumed to ..."

However, the "is assumed" part throws me off. There's no assumption here -- 
this is what this attribute means, intentionally, by design, no guessing 
involved.

So how about this?

The attribute `[[gsl::Owner(T)]]` applies to structs and classes that own an 
object of type `T`:

```
[[gsl::Owner(int)]] class IntOwner {
private:
  int value;
public:
  int *getInt() { return &value; }
  long *getLong();
};
```

In a class annotated like this each member function that returns `T&` or `T*` 
returns a pointer/reference to the data owned by the class instance:

```
IntOwner int_owner;
int *x = int_owner.getInt();
// `x` points to memory owned by `int_owner`.
```

Therefore, that the lifetime of that data ends when ... For example:

```
int *x = IntOwner().getInt();
// `x` points to memory owned by the temporary `IntOwner()`, which has been 
destroyed now.

long *y = IntOwner().getLong();
// Can't make any conclusion about validity of `y`.
```




Comment at: clang/include/clang/Basic/AttrDocs.td:4177
+  let Content = [{
+When annotating a class ``P`` with ``[[gsl::Pointer(T)]]``, it assumed to be a
+non-owning type whose objects can point to ``T`` type objects or dangle.

Ditto, "A class annotated with ... is assumed to be ..."

However, again, I feel like "assume" is the wrong word to use.

"The attribute `[[gsl::Pointer(T)]]` applies to structs and classes that behave 
like pointers to an object of type `T`:

```
class [[gsl::Pointer(int)]] IntPointer {
private:
  int *valuePointer;
public:
  // fill it in with appropriate APIs
};
```

Classes annotated like this behave like regular pointers: they can either point 
to objects of type `T`, or dangle (<<>>).

For example: ... sample code and a static analysis message...



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << 
AL;
+return;

xazax.hun wrote:
> aaron.ballman wrote:
> > Is `void` really the only problematic type? What about incomplete types, 
> > for instance?
> I think incomplete types are fine. The only information we actually need here 
> is the name the pointee types. E.g. for a `SomeOwner` which is 
> annotated `Owner(Incomplete)`, we will assume that a method returning 
> `Incomplete*`, `std::reference_wrapper` etc will return an object 
> pointing to the memory owned by `SomeOwner`.
What about array types? References?

gsl::Owner(int[])
gsl::Owner(int&)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:4177
+  let Content = [{
+When annotating a class ``P`` with ``[[gsl::Pointer(T)]]``, it assumed to be a
+non-owning type whose objects can point to ``T`` type objects or dangle.

aaron.ballman wrote:
> You don't reference `P` in the rest of the docs, is that intentional?
I think it is referenced implicitly after the comma by `it`. Would you prefer 
to get rid of the name `P` and only say `a class`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 207613.
mgehre added a comment.

- Make the list of attribute properties more consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// expected-error@-1 {{'Owner' attribute takes one argument}}
+class [[gsl::Pointer]] PointerMissingParameter{};
+// expected-error@-1 {{'Pointer' attribute takes one argument}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+class [[gsl::Pointer(S)]] APointer{};
+
+class AddOwnerLater {};
+class [[gsl::Owner(int)]] AddOwnerLater;
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddConflictLater2{};
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+class [[gsl::Owner(int)]] AddTheSameLater;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -116,8 +116,10 @@
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -211,6 +211,15 @@
 }
 }
 
+namespace TestLifetimeCategories {
+class [[gsl::Owner(int)]] AOwner{};
+// CHECK: CXXRecordDecl{{.*}} class AOwner
+// CHECK: OwnerAttr {{.*}} int
+class [[gsl::Pointer(int)]] APointer{};
+// CHECK: CXXRecordDecl{{.*}} class APointer
+// CHECK: PointerAttr {{.*}} int
+} // namespace TestLifetimeCategories
+
 // Verify the order of attributes in the Ast. It must reflect the order
 // in the parsed source.
 int mergeAttrTest() __attribute__((deprecated)) __attribute__((warn_unused_result));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAtt

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 207400.
mgehre added a comment.

- Add newline at end of file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// expected-error@-1 {{'Owner' attribute takes one argument}}
+class [[gsl::Pointer]] PointerMissingParameter{};
+// expected-error@-1 {{'Pointer' attribute takes one argument}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+class [[gsl::Pointer(S)]] APointer{};
+
+class AddOwnerLater {};
+class [[gsl::Owner(int)]] AddOwnerLater;
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddConflictLater2{};
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+class [[gsl::Owner(int)]] AddTheSameLater;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -116,8 +116,10 @@
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -211,6 +211,15 @@
 }
 }
 
+namespace TestLifetimeCategories {
+class [[gsl::Owner(int)]] AOwner{};
+// CHECK: CXXRecordDecl{{.*}} class AOwner
+// CHECK: OwnerAttr {{.*}} int
+class [[gsl::Pointer(int)]] APointer{};
+// CHECK: CXXRecordDecl{{.*}} class APointer
+// CHECK: PointerAttr {{.*}} int
+} // namespace TestLifetimeCategories
+
 // Verify the order of attributes in the Ast. It must reflect the order
 // in the parsed source.
 int mergeAttrTest() __attribute__((deprecated)) __attribute__((warn_unused_result));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4533,6 +4533,48 @@

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I understand that putting types on forward declared header (standard libraries 
/ third party libraries) is not a approach we should favor. 
Thank you for your good arguments.
API notes seems like a really good approach for this (I would like to help with 
the upstreaming), and in the meanwhile
I will work on hard-coding attributes for standard library types in clang for a 
follow-up PR. I think we could have them always attached (I will measure 
performance),
so that -Wlifetime will not change the AST.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

In D63954#1565128 , @xazax.hun wrote:

> In D63954#1565109 , @gribozavr wrote:
>
> > > I agree. In a follow-up patch we will add the attributes to STL types 
> > > during parsing. Do you think it is OK to always add the attributes or 
> > > should we only do it if a flag, e.g.  -wlifetime is added to the compiler 
> > > invocation?
> >
> > Warning flags should not change the AST -- compiler engineers don't expect 
> > that. So if Clang is going to perform inference, it should either always do 
> > it, or it should be guarded by an `-f` flag, not a `-W` flag.
>
>
> Thanks, this make sense. My only concern would be that a -W flag without the 
> "inference" for STL types would be useless. Is it ok to make the driver add a 
> -f flag automatically if a warning is turned on or would you find that 
> confusing as well?


Why not always attach attributes to standard library types? I only suggested 
the `-f` flag in case it is necessary to gate this functionality for some 
reason (e.g., performance).

>>> On the other hand I still think it is useful to give the users the option 
>>> to maintain a header with forward declarations to add the annotations to 
>>> other (non-standard) 3rd party types. These headers might be fragile to 3rd 
>>> party changes but could still be a better option than maintaining patches 
>>> on top of 3rd party libraries. Having API notes upstream would be a 
>>> superior solution here and I might look into upstreaming it at some point, 
>>> but I think this is something that could be addressed later on. What do you 
>>> think?
>> 
>> I think it is acceptable for 3rd party libraries, but there's already a 
>> solution for 3rd party libraries -- API notes in Swift's fork of Clang. It 
>> has been successfully used by Apple for 5 years for this exact purpose 
>> (adding attributes to existing libraries without changing headers), and only 
>> needs to be upstreamed to Clang.
> 
> Supporting the forward declarations way is only a few lines of code now, 
> supporting API Notes is a much larger effort. I would also prefer the latter 
> but I think having this work not blocked on upstreaming API notes is 
> essential to get this upstreamed. Or would you prefer not supporting the 3rd 
> party library use-case until API Notes are upstreamed?

I would prefer an API Notes-based approach. And I would suggest that you, or 
someone else working on these attributes, help with upstreaming.

The reason why I prefer API notes is because users forward declaring third 
party APIs will put those libraries in a difficult situation, where those 
libraries can't use normal evolution processes to change their APIs -- the 
forward declarations that users have will stop compiling, or will start 
introducing additional overloads which don't have definitions. Of course 
severity of the problem depends on the nature of such forward declarations -- 
would it only be for owner and pointer types, or for some other APIs? Just the 
types -- and rare types like pointers -- is not that bad, however forward 
declaring functions is bad.

With API notes users have all the extra stuff out of line, and it is easy to 
disable when things go sideways.

When forward declarations are sprinkled in users' code, they are impossible to 
change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 207381.
mgehre marked 5 inline comments as done.
mgehre added a comment.

- Address more review comments.
- git-clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// expected-error@-1 {{'Owner' attribute takes one argument}}
+class [[gsl::Pointer]] PointerMissingParameter{};
+// expected-error@-1 {{'Pointer' attribute takes one argument}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+class [[gsl::Pointer(S)]] APointer{};
+
+class AddOwnerLater {};
+class [[gsl::Owner(int)]] AddOwnerLater;
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddConflictLater2{};
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -116,8 +116,10 @@
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -211,6 +211,15 @@
 }
 }
 
+namespace TestLifetimeCategories {
+class [[gsl::Owner(int)]] AOwner{};
+// CHECK: CXXRecordDecl{{.*}} class AOwner
+// CHECK: OwnerAttr {{.*}} int
+class [[gsl::Pointer(int)]] APointer{};
+// CHECK: CXXRecordDecl{{.*}} class APointer
+// CHECK: PointerAttr {{.*}} int
+} // namespace TestLifetimeCategories
+
 // Verify the order of attributes in the Ast. It must reflect the order
 // in the parsed source.
 int mergeAttrTest() __attribute__((deprecated)) __attribute__((warn_unused_result));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
---

[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D63954#1565109 , @gribozavr wrote:

> > I agree. In a follow-up patch we will add the attributes to STL types 
> > during parsing. Do you think it is OK to always add the attributes or 
> > should we only do it if a flag, e.g.  -wlifetime is added to the compiler 
> > invocation?
>
> Warning flags should not change the AST -- compiler engineers don't expect 
> that. So if Clang is going to perform inference, it should either always do 
> it, or it should be guarded by an `-f` flag, not a `-W` flag.


Thanks, this make sense. My only concern would be that a -W flag without the 
"inference" for STL types would be useless. Is it ok to make the driver add a 
-f flag automatically if a warning is turned on or would you find that 
confusing as well?

> 
> 
>> On the other hand I still think it is useful to give the users the option to 
>> maintain a header with forward declarations to add the annotations to other 
>> (non-standard) 3rd party types. These headers might be fragile to 3rd party 
>> changes but could still be a better option than maintaining patches on top 
>> of 3rd party libraries. Having API notes upstream would be a superior 
>> solution here and I might look into upstreaming it at some point, but I 
>> think this is something that could be addressed later on. What do you think?
> 
> I think it is acceptable for 3rd party libraries, but there's already a 
> solution for 3rd party libraries -- API notes in Swift's fork of Clang. It 
> has been successfully used by Apple for 5 years for this exact purpose 
> (adding attributes to existing libraries without changing headers), and only 
> needs to be upstreamed to Clang.

Supporting the forward declarations way is only a few lines of code now, 
supporting API Notes is a much larger effort. I would also prefer the latter 
but I think having this work not blocked on upstreaming API notes is essential 
to get this upstreamed. Or would you prefer not supporting the 3rd party 
library use-case until API Notes are upstreamed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> I agree. In a follow-up patch we will add the attributes to STL types during 
> parsing. Do you think it is OK to always add the attributes or should we only 
> do it if a flag, e.g.  -wlifetime is added to the compiler invocation?

Warning flags should not change the AST -- compiler engineers don't expect 
that. So if Clang is going to perform inference, it should either always do it, 
or it should be guarded by an `-f` flag, not a `-W` flag.

> On the other hand I still think it is useful to give the users the option to 
> maintain a header with forward declarations to add the annotations to other 
> (non-standard) 3rd party types. These headers might be fragile to 3rd party 
> changes but could still be a better option than maintaining patches on top of 
> 3rd party libraries. Having API notes upstream would be a superior solution 
> here and I might look into upstreaming it at some point, but I think this is 
> something that could be addressed later on. What do you think?

I think it is acceptable for 3rd party libraries, but there's already a 
solution for 3rd party libraries -- API notes in Swift's fork of Clang. It has 
been successfully used by Apple for 5 years for this exact purpose (adding 
attributes to existing libraries without changing headers), and only needs to 
be upstreamed to Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4553
+  // we always add (and check) the attribute to the cannonical decl.
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {

aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > Will this work? What happens if we see a forward declaration with this 
> > > attribute first and then see the canonical declaration later? I suspect 
> > > this work needs to be done when merging declaration attributes instead.
> > For `TagDecl`s the canonical decl is the first declaration:
> > ```
> > TagDecl *TagDecl::getCanonicalDecl() { return getFirstDecl(); }
> > ```
> > 
> > So I suspect we can never see a non-canonical declaration first. And once 
> > we see the canonical declaration it remains the same no matter how many new 
> > declaration do we see.
> This is the scenario I am worried about:
> ```
> struct [[whatever]] Foo;
> struct Foo {};
> ```
This works well as the forward declaration is the canonical declaration since 
it is the first declaration in the redecl chain. A declaration being canonical 
or being a definition are two independent properties.

The only caveat here is that the users of these attributes always need to query 
the attribute of the canonical decl.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4553
+  // we always add (and check) the attribute to the cannonical decl.
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {

xazax.hun wrote:
> aaron.ballman wrote:
> > Will this work? What happens if we see a forward declaration with this 
> > attribute first and then see the canonical declaration later? I suspect 
> > this work needs to be done when merging declaration attributes instead.
> For `TagDecl`s the canonical decl is the first declaration:
> ```
> TagDecl *TagDecl::getCanonicalDecl() { return getFirstDecl(); }
> ```
> 
> So I suspect we can never see a non-canonical declaration first. And once we 
> see the canonical declaration it remains the same no matter how many new 
> declaration do we see.
This is the scenario I am worried about:
```
struct [[whatever]] Foo;
struct Foo {};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D63954#1564448 , @gribozavr wrote:

> > We explicitly allow to add an annotation after the definition of the class 
> > to allow adding annotations to external source from by the user
>
> Asking users to forward-declare anything from the standard library is a very 
> bad idea -- in fact it is undefined behavior. 
> https://stackoverflow.com/questions/307343/forward-declare-an-stl-container
>
> The compiler should just know about standard library types and attach 
> attributes automatically.


I agree. In a follow-up patch we will add the attributes to STL types during 
parsing. Do you think it is OK to always add the attributes or should we only 
do it if a flag, e.g. `-wlifetime` is added to the compiler invocation?

On the other hand I still think it is useful to give the users the option to 
maintain a header with forward declarations to add the annotations to other 
(non-standard) 3rd party types. These headers might be fragile to 3rd party 
changes but could still be a better option than maintaining patches on top of 
3rd party libraries. Having API notes upstream would be a superior solution 
here and I might look into upstreaming it at some point, but I think this is 
something that could be addressed later on. What do you think?




Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;

aaron.ballman wrote:
> Has Microsoft already added support for this? We had an unfortunate problem 
> with `gsl::suppress` where we introduced the attribute first and MSVC 
> introduced it under a different, incompatible syntax. I would strongly like 
> to avoid repeating that.
I will double check this. Her Sutter's paper have this spelling and I believe 
that the Microsoft implementation is following the same spelling. The main 
difference is that the MSVC version does not have a type argument at this point 
but they do plan to add it as an optional argument. (They want to infer the 
pointee type when it is not provided.) The clang community do not want the 
inference, hence we made the type argument required. Always providing the type 
argument should be compatible with future versions of the MSVC implementation.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4564
+  if (Attr->getDerefType().getTypePtr() != ParmType.getTypePtr()) {
+S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible) << AL << 
Attr;
+S.Diag(Attr->getLocation(), diag::note_conflicting_attribute);

erik.pilkington wrote:
> `diag::warn_duplicate_attr`?
We actually allow duplicated attributes as long as they are not conflicting. 
The reason why we introduced a new diagnostic is that the source of the problem 
is the conflict, i.e.: the two attributes are contradicting each other. Not the 
duplication.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4537
+static void handleLifetimeCategoryAttr(Sema &S, Decl *D, const ParsedAttr &AL) 
{
+  if (!AL.hasParsedType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;

aaron.ballman wrote:
> Under what circumstances is this check needed? I believe this is already 
> automatically handled for you.
It is handled automatically indeed, thanks!



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << 
AL;
+return;

aaron.ballman wrote:
> Is `void` really the only problematic type? What about incomplete types, for 
> instance?
I think incomplete types are fine. The only information we actually need here 
is the name the pointee types. E.g. for a `SomeOwner` which is 
annotated `Owner(Incomplete)`, we will assume that a method returning 
`Incomplete*`, `std::reference_wrapper` etc will return an object 
pointing to the memory owned by `SomeOwner`.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4553
+  // we always add (and check) the attribute to the cannonical decl.
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {

aaron.ballman wrote:
> Will this work? What happens if we see a forward declaration with this 
> attribute first and then see the canonical declaration later? I suspect this 
> work needs to be done when merging declaration attributes instead.
For `TagDecl`s the canonical decl is the first declaration:
```
TagDecl *TagDecl::getCanonicalDecl() { return getFirstDecl(); }
```

So I suspect we can never see a non-canonical declaration first. And once we 
see the canonical declaration it remains the same no matter how many new 
declaration do we see.


Repository:
  rG LLVM Github Monorepo

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

https://r

[PATCH] D63954: Add lifetime categories attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> We explicitly allow to add an annotation after the definition of the class to 
> allow adding annotations to external source from by the user

Asking users to forward-declare anything from the standard library is a very 
bad idea -- in fact it is undefined behavior. 
https://stackoverflow.com/questions/307343/forward-declare-an-stl-container

The compiler should just know about standard library types and attach 
attributes automatically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63954#1562928 , @erik.pilkington 
wrote:

> > We explicitly allow to add an annotation after
> >  the definition of the class to allow adding annotations
> >  to external source from by the user, e.g.
> > 
> >   #include 
> >   
> >   namespace std {
> >   template
> >   class [[gsl::Owner(T)]] vector;
> >   }
>
> Wait, does that even work? What if the vector was declared in an inline 
> namespace in the header? Seems like a strange recommendation for users. Is 
> there some reason you can't just add these annotations to standard libraries? 
> I doubt libcxx would have a problem with getting better warnings on their 
> types.


We do not want to encourage users to redeclare things within namespace std like 
this -- I believe that is UB.




Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;

Has Microsoft already added support for this? We had an unfortunate problem 
with `gsl::suppress` where we introduced the attribute first and MSVC 
introduced it under a different, incompatible syntax. I would strongly like to 
avoid repeating that.



Comment at: clang/include/clang/Basic/AttrDocs.td:4167
+pointer/reference to the data owned by ``O``. The owned data is assumed to end
+its lifetime once the owning object's lifetime end.
+

the owning object's lifetime end -> the owning object's lifetime ends



Comment at: clang/include/clang/Basic/AttrDocs.td:4177
+  let Content = [{
+When annotating a class ``P`` with ``[[gsl::Pointer(T)]]``, it assumed to be a
+non-owning type whose objects can point to ``T`` type objects or dangle.

You don't reference `P` in the rest of the docs, is that intentional?



Comment at: clang/include/clang/Basic/AttrDocs.td:4186
+}
\ No newline at end of file


Please keep the newline at the end of the file.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4537
+static void handleLifetimeCategoryAttr(Sema &S, Decl *D, const ParsedAttr &AL) 
{
+  if (!AL.hasParsedType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;

Under what circumstances is this check needed? I believe this is already 
automatically handled for you.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << 
AL;
+return;

Is `void` really the only problematic type? What about incomplete types, for 
instance?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4553
+  // we always add (and check) the attribute to the cannonical decl.
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {

Will this work? What happens if we see a forward declaration with this 
attribute first and then see the canonical declaration later? I suspect this 
work needs to be done when merging declaration attributes instead.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4554
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {
+if (checkAttrMutualExclusion(S, D, AL))

Formatting.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4557
+  return;
+if (const auto *Attr = D->getAttr()) {
+  if (Attr->getDerefType().getTypePtr() != ParmType.getTypePtr()) {

Please use a name other than `Attr` (as that's a type name).



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:61
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file


Please add a newline to the end of the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 2 inline comments as done.
mgehre added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4560-4561
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {
+if (checkAttrMutualExclusion(S, D, AL))
+  return;
+if (const auto *Attr = D->getAttr()) {

erik.pilkington wrote:
> This is duplicated with the first line in the function.
Removed the first line, which only checked for duplicate attributes on the same 
declaration, but we must check (here) that all matching
declaration are consistent - by performing all checks on the canonical 
declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 207235.
mgehre added a comment.

Address some review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+ // expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner]] OwnerMissingParameter {};
+// expected-error@-1 {{'Owner' attribute takes one argument}}
+class [[gsl::Pointer]] PointerMissingParameter {};
+// expected-error@-1 {{'Pointer' attribute takes one argument}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType {};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType {};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer {};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner {};
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer {};
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType {};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType {};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner(int)]] AnOwner {};
+class [[gsl::Pointer(S)]] APointer {};
+
+class AddOwnerLater {};
+class [[gsl::Owner(int)]] AddOwnerLater;
+
+class [[gsl::Pointer(int)]] AddConflictLater {};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddConflictLater2 {};
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddTheSameLater {};
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -116,8 +116,10 @@
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -211,6 +211,15 @@
 }
 }
 
+namespace TestLifetimeCategories {
+  class [[gsl::Owner(int)]] AOwner {};
+  // CHECK: CXXRecordDecl{{.*}} class AOwner
+  // CHECK: OwnerAttr {{.*}} int
+  class [[gsl::Pointer(int)]] APointer {};
+  // CHECK: CXXRecordDecl{{.*}} class APointer
+  // CHECK: PointerAttr {{.*}} int
+}
+
 // Verify the order of attributes in the Ast. It must reflect the order
 // in the parsed source.
 int mergeAttrTest() __attribute__((deprecated)) __attribute__((warn_unused_result));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

We can and should explore both options and see what works best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I actually think we should hard code STL (and maybe some other popular) types 
into the compiler. I.e.: if an STL type is unannotated and the warnings are 
turned on, we could look up the default annotations from a table and append 
them during parsing. This could be done in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

We can try to add the annotations to libcxx (which will still take time),
but there are also other standard libraries (libstdc++, MSVC) that
can be used with clang.
Eventually everybody will have lifetime annotations (of course ;-) ),
but in the meantime there seems to be no other way to enable
the analysis on unmodified STL.

My idea would be to maintain a `lifetime-annotations` headers (outside of clang 
repo?)
with #ifdef magic to add lifetime annotations for major STL variants/versions 
as a drop-in
to enable lifetime analysis. It's not nice, but only temporary and at least 
better than requiring
to change your STL to get lifetime analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

> We explicitly allow to add an annotation after
>  the definition of the class to allow adding annotations
>  to external source from by the user, e.g.
> 
>   #include 
>   
>   namespace std {
>   template
>   class [[gsl::Owner(T)]] vector;
>   }

Wait, does that even work? What if the vector was declared in an inline 
namespace in the header? Seems like a strange recommendation for users. Is 
there some reason you can't just add these annotations to standard libraries? I 
doubt libcxx would have a problem with getting better warnings on their types.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4560-4561
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {
+if (checkAttrMutualExclusion(S, D, AL))
+  return;
+if (const auto *Attr = D->getAttr()) {

This is duplicated with the first line in the function.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4564
+  if (Attr->getDerefType().getTypePtr() != ParmType.getTypePtr()) {
+S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible) << AL << 
Attr;
+S.Diag(Attr->getLocation(), diag::note_conflicting_attribute);

`diag::warn_duplicate_attr`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 207138.
mgehre added a comment.

Fix typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+ // expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner]] OwnerMissingParameter {};
+// expected-error@-1 {{'Owner' attribute takes one argument}}
+class [[gsl::Pointer]] PointerMissingParameter {};
+// expected-error@-1 {{'Pointer' attribute takes one argument}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType {};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType {};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer {};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner {};
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer {};
+// expected-error@-1 {{'Pointer' and 'Pointer' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType {};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType {};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner(int)]] AnOwner {};
+class [[gsl::Pointer(S)]] APointer {};
+
+class AddOwnerLater {};
+class [[gsl::Owner(int)]] AddOwnerLater;
+
+class [[gsl::Pointer(int)]] AddConflictLater {};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddConflictLater2 {};
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddTheSameLater {};
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -116,8 +116,10 @@
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -211,6 +211,15 @@
 }
 }
 
+namespace TestLifetimeCategories {
+  class [[gsl::Owner(int)]] AOwner {};
+  // CHECK: CXXRecordDecl{{.*}} class AOwner
+  // CHECK: OwnerAttr {{.*}} int
+  class [[gsl::Pointer(int)]] APointer {};
+  // CHECK: CXXRecordDecl{{.*}} class APointer
+  // CHECK: PointerAttr {{.*}} int
+}
+
 // Verify the order of attributes in the Ast. It must reflect the order
 // in the parsed source.
 int mergeAttrTest()

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added a reviewer: gribozavr.
Herald added a project: clang.

This is the first part of work announced in 
"[RFC] Adding lifetime analysis to clang" [0],
i.e. the addition of the [[gsl::Owner(T)]] and
[[gsl::Pointer(T)]] attributes, which
will enable user-defined types to participate in
the lifetime analysis (which will be part of the
next PR).
The type `T` here is called "DerefType" in the paper,
and denotes the type that an Owner owns and a Pointer
points to. E.g. `std::vector` should be annotated
with `[[gsl::Owner(int)]]` and 
a `std::vector::iterator` with `[[gsl::Pointer(int)]]`.

We explicitly allow to add an annotation after
the definition of the class to allow adding annotations
to external source from by the user, e.g.

  #include 
  
  namespace std {
  template
  class [[gsl::Owner(T)]] vector;
  }

until we can ship a standard library with annotations.

[0] http://lists.llvm.org/pipermail/cfe-dev/2018-November/060355.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+ // expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner]] OwnerMissingParameter {};
+// expected-error@-1 {{'Owner' attribute takes one argument}}
+class [[gsl::Pointer]] PointerMissingParameter {};
+// expected-error@-1 {{'Pointer' attribute takes one argument}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType {};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType {};
+// expected-error@-1 {{expected a type}} expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer {};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner {};
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer {};
+// expected-error@-1 {{'Pointer' and 'Pointer' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType {};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType {};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner(int)]] AnOwner {};
+class [[gsl::Pointer(S)]] APointer {};
+
+class AddOwnerLater {};
+class [[gsl::Owner(int)]] AddOwnerLater;
+
+class [[gsl::Pointer(int)]] AddConflictLater {};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddConflictLater2 {};
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-3 {{conflicting attribute is here}}
+
+class [[gsl::Owner(int)]] AddTheSameLater {};
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -116,8 +116,10 @@
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (Subject