[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368147: gsl::Owner/gsl::Pointer: Add implicit annotations 
for some std types (authored by mgehre, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64448?vs=212446=213836#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64448

Files:
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaAttr.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaTemplate.cpp
  cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -6116,6 +6116,17 @@
   ClassTemplateSpecializationDecl *BaseTemplateSpec,
   SourceLocation BaseLoc);
 
+  /// Add gsl::Pointer attribute to std::container::iterator
+  /// \param ND The declaration that introduces the name
+  /// std::container::iterator. \param UnderlyingRecord The record named by ND.
+  void inferGslPointerAttribute(NamedDecl *ND, CXXRecordDecl *UnderlyingRecord);
+
+  /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types.
+  void inferGslOwnerPointerAttribute(CXXRecordDecl *Record);
+
+  /// Add [[gsl::Pointer]] attributes for std:: types.
+  void inferGslPointerAttribute(TypedefNameDecl *TD);
+
   void CheckCompletedCXXClass(CXXRecordDecl *Record);
 
   /// Check that the C++ class annoated with "trivial_abi" satisfies all the
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -4249,7 +4249,7 @@
 int *getInt() { return  }
   };
 
-The argument ``T`` is optional and currently ignored.
+The argument ``T`` is optional and is ignored.
 This attribute may be used by analysis tools and has no effect on code
 generation.
 
@@ -4275,7 +4275,7 @@
 int *getInt() { return  }
   };
 
-The argument ``T`` is optional and currently ignored.
+The argument ``T`` is optional and is ignored.
 This attribute may be used by analysis tools and has no effect on code
 generation.
 
Index: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
+++ cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Sema/Sema.h:6122
+  /// std::container::iterator. \param UnderlyingRecord The record named by ND.
+  void addDefaultGslPointerAttribute(NamedDecl *ND,
+ CXXRecordDecl *UnderlyingRecord);

s/addDefault/infer/ ?  (everywhere in the patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

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

- Fix crash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1690,6 +1690,7 @@
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   AddPushedVisibilityAttribute(NewClass);
+  addDefaultGslOwnerPointerAttribute(NewClass);
 
  

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

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

- Add missing check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1690,6 +1690,7 @@
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   AddPushedVisibilityAttribute(NewClass);
+  

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:6097
+
+  /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types.
+  void addDefaultGslPointerAttribute(TypedefNameDecl *TD);

gribozavr wrote:
> It seems like this function does not add gsl::Owner.
Fixed comment



Comment at: clang/lib/Sema/SemaTemplate.cpp:1689
   AddPushedVisibilityAttribute(NewClass);
+  addDefaultGslOwnerPointerAttribute(NewClass);
 

gribozavr wrote:
> It shouldn't be necessary to perform inference here, instead, the attributes 
> should be instantiated, see `instantiateDependentAlignedAttr` in 
> SemaTemplateInstantiateDecl.cpp for an example.
From what I understand, here the attribute is put on the `ClassTemplateDecl`. 
Without this, there would be no attribute to instantiate from?
The instantiation of OwnerAttr/PointerAttr is auto-generated into 
instantiateTemplateAttribute() in 
`build/tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc`.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:702
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);
+  SemaRef.addDefaultGslPointerAttribute(Typedef);
 

gribozavr wrote:
> Ditto, should not be necessary to perform inference here.
I'll move this to already act on the typedef in the ClassTemplateDecl (instead 
of the instantiation here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

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

- Fix comments
- Add Pointer via typedef on ClassTemplateDecl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1690,6 +1690,7 @@
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:6097
+
+  /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types.
+  void addDefaultGslPointerAttribute(TypedefNameDecl *TD);

It seems like this function does not add gsl::Owner.



Comment at: clang/lib/Sema/SemaTemplate.cpp:1689
   AddPushedVisibilityAttribute(NewClass);
+  addDefaultGslOwnerPointerAttribute(NewClass);
 

It shouldn't be necessary to perform inference here, instead, the attributes 
should be instantiated, see `instantiateDependentAlignedAttr` in 
SemaTemplateInstantiateDecl.cpp for an example.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:702
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);
+  SemaRef.addDefaultGslPointerAttribute(Typedef);
 

Ditto, should not be necessary to perform inference here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D64448#1595022 , @mgehre wrote:

> In D64448#1595017 , @lebedev.ri 
> wrote:
>
> > Just passing-by thought:
> >  These attributes that are being added implicitly, it will be possible to 
> > differentiate
> >  whether an attribute was actually present in the code, or was added 
> > implicitly,
> >  say for clang-tidy check purposes?
> >  Is there some 'implicit' bit on these, or they have a location not in the 
> > source buffer?
>
>
> Thanks for the question! The implicitly added attributes have an invalid 
> source location.


Sounds good, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In D64448#1595017 , @lebedev.ri wrote:

> Just passing-by thought:
>  These attributes that are being added implicitly, it will be possible to 
> differentiate
>  whether an attribute was actually present in the code, or was added 
> implicitly,
>  say for clang-tidy check purposes?
>  Is there some 'implicit' bit on these, or they have a location not in the 
> source buffer?


Thanks for the question! The implicitly added attributes have an invalid source 
location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

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

- Remove type traits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -699,6 +699,7 @@
   }
 
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);
+  SemaRef.addDefaultGslPointerAttribute(Typedef);
 
   Typedef->setAccess(D->getAccess());
 
Index: 

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to 
differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the 
source buffer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

This should fix all open comments.




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

rsmith wrote:
> ... and what does the attribute mean when the argument is omitted?
I added text that it is currently ignored. We will use the argument in future 
analysis, but already add parsing of it for forward compatibility.
Per Aaron's request, this part moved to the parent PR, D63954.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {

mgehre wrote:
> gribozavr wrote:
> > mgehre wrote:
> > > gribozavr wrote:
> > > > I feel like these tests would be better off in a separate file.
> > > > 
> > > > It would be also good to explain which exact library we are trying to 
> > > > imitate in this test. Different libraries use different coding patterns.
> > > This is imitating techniques from different libraries - all techniques 
> > > that this implementation supports.
> > > 
> > > To check if all techniques that this implementation supports are enough 
> > > for real standard library implementations,
> > > I use 
> > > https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
> > >  against them. Various versions of libstdc++
> > > and libc++ passed. I will test MSVC standard library next. If they would 
> > > use a technique that this implementation does not support yet,
> > > I will add support for that and the corresponding test here.
> > > I might fix MSVC support (if needed) only in the following PR:
> > > This is imitating techniques from different libraries - all techniques 
> > > that this implementation supports.
> > 
> > Okay -- please add comments about each technique then (and ideally, which 
> > libraries use them). Right now (for me, who didn't write the patch), the 
> > test looks like it is testing inference for a bunch of types, not for a 
> > bunch of techniques -- the differences are subtle and non-obvious.
> Sure, I will add comments.
I moved the `std` tests to a different file.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:93
 
-// Complete template
+// Attributes are added to a instantiatons of a complete template.
 template 

gribozavr wrote:
> Sorry, but what do you mean by a "concrete template"?
> 
> "Attributes are inferred on class template instantiations."
It says "complete", and I mean "not forward declared".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 211009.
mgehre marked 12 inline comments as done.
mgehre added a comment.

- Merge branch 'lifetime-categories' into hardcode-lifetime-categories; Split 
test; Move test to AST dump; fix for comments;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Basic/TypeTraits.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added subscribers: aaron.ballman, rsmith.
rsmith added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2773
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let MeaningfulToClassTemplateDefinition = 1;
   let Documentation = [LifetimeOwnerDocs];

On the surface this doesn't appear to make sense, but I think that's a 
pre-existing bug.

@aaron.ballman Is this field name reversed from the intent? I think what it 
means is "meaningful on (non-defining) class declaration", which is... the 
opposite of what its name suggests. Looking at the tablegen implementation, it 
appears that setting this to 1 causes the attribute to be instantiated when 
instantiating a class declaration, not only when instantiating a class 
definition.



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

... and what does the attribute mean when the argument is omitted?



Comment at: clang/include/clang/Basic/AttrDocs.td:4180
 non-owning type whose objects can point to ``T`` type objects or dangle.
+The argument ``T`` is optional.
+

Similarly, what happens in that case?



Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > mgehre wrote:
> > > > gribozavr wrote:
> > > > > I'd suggest to split type trait implementations into a separate patch.
> > > > > 
> > > > > Are these type traits being exposed only for testing? I'm not sure it 
> > > > > is a good idea to do that -- people will end up using them in 
> > > > > production code -- is that a concern? If so, maybe it would be better 
> > > > > to test through warnings.
> > > > I copied that approach from https://reviews.llvm.org/D50119.
> > > > If people do it in production code, then at least the two leading 
> > > > underscores should tell them "I'm not supposed to use this".
> > > > 
> > > > I considered a test through warnings, but how would I trigger them? Add 
> > > > `-Wlifetime-categories-debug` which emits notes whenever a 
> > > > [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?
> > > > If people do it in production code, then at least the two leading 
> > > > underscores should tell them "I'm not supposed to use this".
> > > 
> > > That's not what two underscores mean. These custom type traits, being 
> > > language extensions, must have a name that won't collide with any 
> > > user-defined name, hence two underscores. Two underscores mean nothing 
> > > about whether the user is allowed to use it or not. Sure the code might 
> > > become non-portable to other compilers, but that's not what the concern 
> > > is. My concern is that code that people write might become unportable to 
> > > future versions of Clang, where we would have to change behavior of these 
> > > type traits (or it would just subtly change in some corner case and we 
> > > won't notice since we don't consider it a public API).
> > > 
> > > > I considered a test through warnings, but how would I trigger them?
> > > 
> > > I meant regular warnings, that are the objective of this analysis -- 
> > > warnings about dereferencing dangling pointers. If we get those warnings 
> > > for the types-under-test, then obviously they have the correct 
> > > annotations from the compiler's point of view.
> > I spent a lot of time debugging on the full lifetime analysis, and knowing 
> > exactly which type has which Attribute (especially if inference is 
> > involved) was quite important.
> > I would say that adding additional code to trigger a real lifetime warning 
> > - requires that we first add some lifetime warnings to clang (currently in 
> > a different PR)
> > - obscures the purpose of the check, i.e. checking the attributes and not 
> > the warnings themselves
> > - and makes debugging hard when the test fails (warnings involve at least 
> > one Owner and one Pointer, so either of those could have made the test fail)
> > I'd prefer if we can test the attributes directly.
> I agree that being able to test these properties definitely simplifies 
> testing. I am worried about adding language extension though, and would like 
> someone like Richard Smith to LGTM this approach.
If you just want this for testing, could you instead use an `-ast-dump` test 
and see if the attributes were added? Alternatively I'd be OK having these as 
just-for-testing traits if you give them names that makes their status as being 
for-testing, unsupported, may-be-removed-or-changed-at-any-time clear. (Eg, 

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

In D64448#1581866 , @mgehre wrote:

> I didn't know whether they would differ, but the test tells me that they 
> don't differ significantly (i.e. in introducing new techniques).


Okay -- I would prefer if you intentionally looked at the code before declaring 
a library as supported, or asked someone who knows about the differences, but 
it is your call.

> Would you like me to test with other standard libraries (besides MSVC, which 
> I already planned)?

It is your call which libraries you would like to support.

Another question -- in the latest iteration of the patch the inference code 
disappeared -- was it intentional?




Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

mgehre wrote:
> gribozavr wrote:
> > mgehre wrote:
> > > gribozavr wrote:
> > > > I'd suggest to split type trait implementations into a separate patch.
> > > > 
> > > > Are these type traits being exposed only for testing? I'm not sure it 
> > > > is a good idea to do that -- people will end up using them in 
> > > > production code -- is that a concern? If so, maybe it would be better 
> > > > to test through warnings.
> > > I copied that approach from https://reviews.llvm.org/D50119.
> > > If people do it in production code, then at least the two leading 
> > > underscores should tell them "I'm not supposed to use this".
> > > 
> > > I considered a test through warnings, but how would I trigger them? Add 
> > > `-Wlifetime-categories-debug` which emits notes whenever a 
> > > [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?
> > > If people do it in production code, then at least the two leading 
> > > underscores should tell them "I'm not supposed to use this".
> > 
> > That's not what two underscores mean. These custom type traits, being 
> > language extensions, must have a name that won't collide with any 
> > user-defined name, hence two underscores. Two underscores mean nothing 
> > about whether the user is allowed to use it or not. Sure the code might 
> > become non-portable to other compilers, but that's not what the concern is. 
> > My concern is that code that people write might become unportable to future 
> > versions of Clang, where we would have to change behavior of these type 
> > traits (or it would just subtly change in some corner case and we won't 
> > notice since we don't consider it a public API).
> > 
> > > I considered a test through warnings, but how would I trigger them?
> > 
> > I meant regular warnings, that are the objective of this analysis -- 
> > warnings about dereferencing dangling pointers. If we get those warnings 
> > for the types-under-test, then obviously they have the correct annotations 
> > from the compiler's point of view.
> I spent a lot of time debugging on the full lifetime analysis, and knowing 
> exactly which type has which Attribute (especially if inference is involved) 
> was quite important.
> I would say that adding additional code to trigger a real lifetime warning 
> - requires that we first add some lifetime warnings to clang (currently in a 
> different PR)
> - obscures the purpose of the check, i.e. checking the attributes and not the 
> warnings themselves
> - and makes debugging hard when the test fails (warnings involve at least one 
> Owner and one Pointer, so either of those could have made the test fail)
> I'd prefer if we can test the attributes directly.
I agree that being able to test these properties definitely simplifies testing. 
I am worried about adding language extension though, and would like someone 
like Richard Smith to LGTM this approach.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
 
-// Test builtin annotation for std types.
+// Test builtin attributes for std types.
 namespace std {

s/builtin attributes/Test attribute inference for types in the standard 
library./



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:88
 namespace std {
-// Complete class
+// Attributes are added to a (complete) class.
 class any {

s/added to/inferred for/



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:93
 
-// Complete template
+// Attributes are added to a instantiatons of a complete template.
 template 

Sorry, but what do you mean by a "concrete template"?

"Attributes are inferred on class template instantiations."



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:102
 
+// If std::container::iterator is a using declaration, Attributes are added to
+// the underlying class

lowercase "attributes"



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:114
 
+// If 

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In D64448#1581771 , @gribozavr wrote:

> In D64448#1581719 , @mgehre wrote:
>
> > In D64448#159 , @gribozavr 
> > wrote:
> >
> > > I don't know how various versions of libstdc++ differ.
> >
> >
> > The current implementation passed the (partial) test case
> >  
> > https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
> >  for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, 
> > libstdc++ 4.6.4, libstdc++ 4.8.5,
> >  libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
> >  libstdc++ 8.3.0 and libstdc++ 9.1.0.
>
>
> Yes, I saw the testcase -- but how do different libstdc++ versions differ?


I didn't know whether they would differ, but the test tells me that they don't 
differ significantly (i.e. in introducing new techniques). 
Would you like me to test with other standard libraries (besides MSVC, which I 
already planned)?




Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > I'd suggest to split type trait implementations into a separate patch.
> > > 
> > > Are these type traits being exposed only for testing? I'm not sure it is 
> > > a good idea to do that -- people will end up using them in production 
> > > code -- is that a concern? If so, maybe it would be better to test 
> > > through warnings.
> > I copied that approach from https://reviews.llvm.org/D50119.
> > If people do it in production code, then at least the two leading 
> > underscores should tell them "I'm not supposed to use this".
> > 
> > I considered a test through warnings, but how would I trigger them? Add 
> > `-Wlifetime-categories-debug` which emits notes whenever a 
> > [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?
> > If people do it in production code, then at least the two leading 
> > underscores should tell them "I'm not supposed to use this".
> 
> That's not what two underscores mean. These custom type traits, being 
> language extensions, must have a name that won't collide with any 
> user-defined name, hence two underscores. Two underscores mean nothing about 
> whether the user is allowed to use it or not. Sure the code might become 
> non-portable to other compilers, but that's not what the concern is. My 
> concern is that code that people write might become unportable to future 
> versions of Clang, where we would have to change behavior of these type 
> traits (or it would just subtly change in some corner case and we won't 
> notice since we don't consider it a public API).
> 
> > I considered a test through warnings, but how would I trigger them?
> 
> I meant regular warnings, that are the objective of this analysis -- warnings 
> about dereferencing dangling pointers. If we get those warnings for the 
> types-under-test, then obviously they have the correct annotations from the 
> compiler's point of view.
I spent a lot of time debugging on the full lifetime analysis, and knowing 
exactly which type has which Attribute (especially if inference is involved) 
was quite important.
I would say that adding additional code to trigger a real lifetime warning 
- requires that we first add some lifetime warnings to clang (currently in a 
different PR)
- obscures the purpose of the check, i.e. checking the attributes and not the 
warnings themselves
- and makes debugging hard when the test fails (warnings involve at least one 
Owner and one Pointer, so either of those could have made the test fail)
I'd prefer if we can test the attributes directly.



Comment at: clang/test/AST/ast-dump-attr.cpp:222
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > This test and related code changes could be split off into a separate 
> > > patch.
> > I was thinking whether it made sense to separate
> > - fixing the AST dump of attributes with optional type parameter when there 
> > is not such attribute
> > - introduce and attribute with optional type parameter while AST dumping it 
> > is broken
> > so I decided that both are closely related. Otherwise the fix and its test 
> > are in separate PRs?
> Totally makes sense to have the fix and the test is the same PR, but they 
> seem to be separable from attribute inference for std types, right?
Yes, you are right. And Aaron would like to have the type parameter already 
optional in D63954, so I will move this part over there.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace 

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 209370.
mgehre marked 9 inline comments as done.
mgehre added a comment.

- Implement comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -83,14 +83,14 @@
 static_assert(__is_gsl_pointer(int &), "");
 static_assert(__is_gsl_pointer(int *), "");
 
-// Test builtin annotation for std types.
+// Test builtin attributes for std types.
 namespace std {
-// Complete class
+// Attributes are added to a (complete) class.
 class any {
 };
 static_assert(__is_gsl_owner(any), "");
 
-// Complete template
+// Attributes are added to a instantiatons of a complete template.
 template 
 class vector {
 public:
@@ -99,45 +99,60 @@
 static_assert(__is_gsl_owner(vector), "");
 static_assert(__is_gsl_pointer(vector::iterator), "");
 
+// If std::container::iterator is a using declaration, Attributes are added to
+// the underlying class
 template 
-class set_iterator {};
+class __set_iterator {};
 
 template 
 class set {
 public:
-  using iterator = set_iterator;
+  using iterator = __set_iterator;
 };
 static_assert(__is_gsl_pointer(set::iterator), "");
 
+// If std::container::iterator is a typedef, Attributes are added to the
+// underlying class. Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
 template 
-class map_iterator {};
+class __map_iterator {};
 
 template 
 class map {
 public:
-  typedef map_iterator iterator;
+  typedef __map_iterator iterator;
 };
+} // namespace inlinens
 static_assert(__is_gsl_pointer(map::iterator), "");
 
-// list has an implicit gsl::Owner attribute,
+// std::list has an implicit gsl::Owner attribute,
 // but explicit attributes take precedence.
 template 
 class [[gsl::Pointer]] list{};
 static_assert(!__is_gsl_owner(list), "");
 static_assert(__is_gsl_pointer(list), "");
 
-// Forward declared template
+// Forward declared template (Owner)
 template <
 class CharT,
 class Traits>
 class basic_regex;
 static_assert(__is_gsl_owner(basic_regex), "");
 
+// Forward declared template (Pointer)
 template 
 class reference_wrapper;
 static_assert(__is_gsl_pointer(reference_wrapper), "");
 
-class thread;
-static_assert(!__is_gsl_pointer(thread), "");
-static_assert(!__is_gsl_owner(thread), "");
+class some_unknown_type;
+static_assert(!__is_gsl_pointer(some_unknown_type), "");
+static_assert(!__is_gsl_owner(some_unknown_type), "");
 } // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't add implicit attributes.
+class any {
+};
+static_assert(!__is_gsl_owner(any), "");
+} // namespace user
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -4164,7 +4164,7 @@
 When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
 that returns cv-qualified ``T&`` or ``T*`` is assumed to return a
 pointer/reference to the data owned by ``O``. The owned data is assumed to end
-its lifetime once the owning object's lifetime ends. The argument is ``T`` is
+its lifetime once the owning object's lifetime ends. The argument ``T`` is
 optional.
 
 This attribute may be used by analysis tools but will not have effect on code
@@ -4177,7 +4177,7 @@
   let Content = [{
 When annotating a class with ``[[gsl::Pointer(T)]]``, it assumed to be a
 non-owning type whose objects can point to ``T`` type objects or dangle.
-The argument is ``T`` is optional.
+The argument ``T`` is optional.
 
 This attribute may be used by analysis tools but will not have effect on code
 generation.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

mgehre wrote:
> gribozavr wrote:
> > I'd suggest to split type trait implementations into a separate patch.
> > 
> > Are these type traits being exposed only for testing? I'm not sure it is a 
> > good idea to do that -- people will end up using them in production code -- 
> > is that a concern? If so, maybe it would be better to test through warnings.
> I copied that approach from https://reviews.llvm.org/D50119.
> If people do it in production code, then at least the two leading underscores 
> should tell them "I'm not supposed to use this".
> 
> I considered a test through warnings, but how would I trigger them? Add 
> `-Wlifetime-categories-debug` which emits notes whenever a 
> [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?
> If people do it in production code, then at least the two leading underscores 
> should tell them "I'm not supposed to use this".

That's not what two underscores mean. These custom type traits, being language 
extensions, must have a name that won't collide with any user-defined name, 
hence two underscores. Two underscores mean nothing about whether the user is 
allowed to use it or not. Sure the code might become non-portable to other 
compilers, but that's not what the concern is. My concern is that code that 
people write might become unportable to future versions of Clang, where we 
would have to change behavior of these type traits (or it would just subtly 
change in some corner case and we won't notice since we don't consider it a 
public API).

> I considered a test through warnings, but how would I trigger them?

I meant regular warnings, that are the objective of this analysis -- warnings 
about dereferencing dangling pointers. If we get those warnings for the 
types-under-test, then obviously they have the correct annotations from the 
compiler's point of view.



Comment at: clang/test/AST/ast-dump-attr.cpp:222
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument

mgehre wrote:
> gribozavr wrote:
> > This test and related code changes could be split off into a separate patch.
> I was thinking whether it made sense to separate
> - fixing the AST dump of attributes with optional type parameter when there 
> is not such attribute
> - introduce and attribute with optional type parameter while AST dumping it 
> is broken
> so I decided that both are closely related. Otherwise the fix and its test 
> are in separate PRs?
Totally makes sense to have the fix and the test is the same PR, but they seem 
to be separable from attribute inference for std types, right?



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {

mgehre wrote:
> gribozavr wrote:
> > I feel like these tests would be better off in a separate file.
> > 
> > It would be also good to explain which exact library we are trying to 
> > imitate in this test. Different libraries use different coding patterns.
> This is imitating techniques from different libraries - all techniques that 
> this implementation supports.
> 
> To check if all techniques that this implementation supports are enough for 
> real standard library implementations,
> I use 
> https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
>  against them. Various versions of libstdc++
> and libc++ passed. I will test MSVC standard library next. If they would use 
> a technique that this implementation does not support yet,
> I will add support for that and the corresponding test here.
> I might fix MSVC support (if needed) only in the following PR:
> This is imitating techniques from different libraries - all techniques that 
> this implementation supports.

Okay -- please add comments about each technique then (and ideally, which 
libraries use them). Right now (for me, who didn't write the patch), the test 
looks like it is testing inference for a bunch of types, not for a bunch of 
techniques -- the differences are subtle and non-obvious.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:103
+template 
+class set_iterator {};
+

mgehre wrote:
> gribozavr wrote:
> > Is it actually defined like that?
> There might be a standard library implementation that does it like this. This 
> tests that we will use the `using iterator = set_iterator;` in `class set 
> {` to attach the [[gsl::Pointer]] to the `set_iterator`. 
Then std::set_iterator must have an implementation-reserved name, like 
std::__set_iterator.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

In D64448#1581719 , @mgehre wrote:

> In D64448#159 , @gribozavr wrote:
>
> > I don't know how various versions of libstdc++ differ.
>
>
> The current implementation passed the (partial) test case
>  
> https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
>  for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, 
> libstdc++ 4.6.4, libstdc++ 4.8.5,
>  libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
>  libstdc++ 8.3.0 and libstdc++ 9.1.0.


Yes, I saw the testcase -- but how do different libstdc++ versions differ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 5 inline comments as done.
mgehre added a comment.

In D64448#159 , @gribozavr wrote:

> For example, libc++ wraps everything in std in an inline namespace.


I believed that I had written a test for inline namespaces, but seems that I 
shouldn't be working in the middle of the night.
I will add one.

In D64448#159 , @gribozavr wrote:

> I don't know how various versions of libstdc++ differ.


The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, 
libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.




Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

gribozavr wrote:
> I'd suggest to split type trait implementations into a separate patch.
> 
> Are these type traits being exposed only for testing? I'm not sure it is a 
> good idea to do that -- people will end up using them in production code -- 
> is that a concern? If so, maybe it would be better to test through warnings.
I copied that approach from https://reviews.llvm.org/D50119.
If people do it in production code, then at least the two leading underscores 
should tell them "I'm not supposed to use this".

I considered a test through warnings, but how would I trigger them? Add 
`-Wlifetime-categories-debug` which emits notes whenever a 
[[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?



Comment at: clang/test/AST/ast-dump-attr.cpp:222
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument

gribozavr wrote:
> This test and related code changes could be split off into a separate patch.
I was thinking whether it made sense to separate
- fixing the AST dump of attributes with optional type parameter when there is 
not such attribute
- introduce and attribute with optional type parameter while AST dumping it is 
broken
so I decided that both are closely related. Otherwise the fix and its test are 
in separate PRs?



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {

gribozavr wrote:
> I feel like these tests would be better off in a separate file.
> 
> It would be also good to explain which exact library we are trying to imitate 
> in this test. Different libraries use different coding patterns.
This is imitating techniques from different libraries - all techniques that 
this implementation supports.

To check if all techniques that this implementation supports are enough for 
real standard library implementations,
I use 
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp 
against them. Various versions of libstdc++
and libc++ passed. I will test MSVC standard library next. If they would use a 
technique that this implementation does not support yet,
I will add support for that and the corresponding test here.
I might fix MSVC support (if needed) only in the following PR:



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:103
+template 
+class set_iterator {};
+

gribozavr wrote:
> Is it actually defined like that?
There might be a standard library implementation that does it like this. This 
tests that we will use the `using iterator = set_iterator;` in `class set {` 
to attach the [[gsl::Pointer]] to the `set_iterator`. 



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:140
+
+class thread;
+static_assert(!__is_gsl_pointer(thread), "");

gribozavr wrote:
> Unclear what this test is testing.
> 
> If there's something special about thread (e.g., it looks very much like an 
> owner or a pointer, and a buggy implementation can easily declare thread to 
> be an owner or a pointer), please explain that in a comment.
> 
> If you're testing that some random name is not being picked up by inference 
> rules, I'd suggest to use an obviously-fictional name ("class 
> type_unknown_to_compiler;")
Agreed, I will rename this to an obviously-fictional name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Just as Sidenote in case you were not aware: There is a check in clang-tidy for 
`gsl::owner` 
(https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-owning-memory.html)
It will benefit from your annotation and can be simplified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp.

I see. Sorry, but that seems insufficient to me -- different libraries use 
different patterns. For example, libc++ wraps everything in std in an inline 
namespace. I don't know how various versions of libstdc++ differ.




Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

I'd suggest to split type trait implementations into a separate patch.

Are these type traits being exposed only for testing? I'm not sure it is a good 
idea to do that -- people will end up using them in production code -- is that 
a concern? If so, maybe it would be better to test through warnings.



Comment at: clang/test/AST/ast-dump-attr.cpp:222
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument

This test and related code changes could be split off into a separate patch.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {

I feel like these tests would be better off in a separate file.

It would be also good to explain which exact library we are trying to imitate 
in this test. Different libraries use different coding patterns.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:103
+template 
+class set_iterator {};
+

Is it actually defined like that?



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:140
+
+class thread;
+static_assert(!__is_gsl_pointer(thread), "");

Unclear what this test is testing.

If there's something special about thread (e.g., it looks very much like an 
owner or a pointer, and a buggy implementation can easily declare thread to be 
an owner or a pointer), please explain that in a comment.

If you're testing that some random name is not being picked up by inference 
rules, I'd suggest to use an obviously-fictional name ("class 
type_unknown_to_compiler;")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

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

xazax.hun added a comment.

In D64448#1577109 , @gribozavr wrote:

> > The tests are currently here
> >  I think due to their dependency on a standard library, they are not a good 
> > fit for clang/test/. Where else could I put them?
>
> Make some mocks that reproduce the salient parts of different libraries, the 
> coding patterns they use, and check them into clang/test.


Those are already there in `clang/test/SemaCXX/attr-gsl-owner-pointer.cpp`. I 
was thinking about adding the corresponding tests to libc++ as well so we have 
better chances to get some notifications if a new language 
feature/implementation trick is introduced.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> The tests are currently here
>  I think due to their dependency on a standard library, they are not a good 
> fit for clang/test/. Where else could I put them?

Make some mocks that reproduce the salient parts of different libraries, the 
coding patterns they use, and check them into clang/test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: gribozavr, xazax.hun.
Herald added a subscriber: rnkovacs.
Herald added a project: clang.

Make the DerefType, i.e. the argument of gsl::Owner/gsl::Pointer optional for 
now.

The DerefType is used when infering lifetime annotations of functions, e.g.
how a return value can relate to the arguments. Our initial checks should
work without that kind of inference and instead use explicit annotations,
so we don't need the DerefType right now.
And having it optional makes the implicit/default annotations for std:: types
easier to implement. I think the DerefType is also optional in the current MSVC
implementation.

This seems to be the first attribute that has an optional Type argument,
so I needed to fix two thinks in tablegen:

- Don't crash AST dump when Owner/Pointer has no Type argument
- Don't crash when pretty-printing an Attribute with optional type argument

Add __is_gsl_owner/__is_gsl_pointer builtins. They are very useful to test
if the annotations have been attached correctly.

Hard code gsl::Owner/gsl::Pointer for std types. The paper mentions
some types explicitly. Generally, all containers and their iterators are
covered. For iterators, we cover both the case that they are defined
as an nested class or as an typedef/using. I have started to test this
implementation against some real standard library implementations, namely
libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

The tests are currently here

  https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.sh
  https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp

I think due to their dependency on a standard library, they are not a good fit
for clang/test/. Where else could I put them?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64448

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Basic/TypeTraits.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.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
===
--- clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -10,14 +10,15 @@
 
 struct S {
 };
+static_assert(!__is_gsl_owner(S), "");
+static_assert(!__is_gsl_pointer(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 ')'}}
@@ -32,8 +33,12 @@
 // expected-note@-2 {{conflicting attribute is here}}
 
 class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+static_assert(__is_gsl_owner(DuplicateOwner), "");
+static_assert(!__is_gsl_pointer(DuplicateOwner), "");
 
 class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+static_assert(!__is_gsl_owner(DuplicatePointer), "");
+static_assert(__is_gsl_pointer(DuplicatePointer), "");
 
 class [[gsl::Owner(void)]] OwnerVoidDerefType{};
 // expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
@@ -41,7 +46,12 @@
 // expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
 
 class