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

https://reviews.llvm.org/D64448