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

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

This should fix all open comments.



================
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.
 
----------------
rsmith wrote:
> ... and what does the attribute mean when the argument is omitted?
I added text that it is currently ignored. We will use the argument in future analysis, but already add parsing of it for forward compatibility.
Per Aaron's request, this part moved to the parent PR, D63954.


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {
----------------
mgehre wrote:
> gribozavr wrote:
> > 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.
> Sure, I will add comments.
I moved the `std` tests to a different file.


================
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>
----------------
gribozavr wrote:
> Sorry, but what do you mean by a "concrete template"?
> 
> "Attributes are inferred on class template instantiations."
It says "complete", and I mean "not forward declared".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448