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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
