[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#1577779 <https://reviews.llvm.org/D64448#1577779>, @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#1577779 <https://reviews.llvm.org/D64448#1577779>, @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 <typename T>
+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<T>;` 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