[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, `__clang_debug_is_gsl_owner`.)

Generally we would like the `__is_*` type traits to exactly match the semantics of the corresponding standard C++ `std::is_*` type traits (except where we're forced to do something else for compatibility).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448