gribozavr added a comment. In D64448#1581866 <https://reviews.llvm.org/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 <typename T> ---------------- 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 std::container::iterator is a typedef, Attributes are added to the +// underlying class. Inline namespaces are ignored when checking if ---------------- lowercase "attributes" ================ Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:115 +// 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. ---------------- Could you separate the inline namespace test from the iterator part into another test, say, for unordered_map? 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
