[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

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