[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 5 inline comments as done.
xazax.hun added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6510
     LifetimeBoundCall,
+    LifetimePointerInit,
+    LifetimeTempOwner
----------------
gribozavr wrote:
> What is this name supposed to mean? Initialization of a "lifetime pointer"? What's a "lifetime pointer"?
> 
> If you were imitating "LifetimeBoundCall", it is a reference to the attribute: https://clang.llvm.org/docs/AttributeReference.html#lifetimebound .
When we were working on the lifetime analysis we needed to distinguish raw pointers from the user defined types that are categorized as pointers. We adopted this notion of Lifetime Pointer which stands for the user defined type that is annotated with gsl::Pointer.

In case this is confusing I could rename it `GslPointerInit` and `GslTempOwner`. What do you think?


================
Comment at: clang/lib/Sema/SemaInit.cpp:7049
 
+    // Skipping a chain of initializing lifetime Pointer objects.
+    // We are looking only for the final value to find out if it was
----------------
gribozavr wrote:
> Sorry, I can't parse this: "... initializing lifetime Pointer ..."
We called gsl::Pointer annotated types lifetime pointers (to distinguish from raw pointers). Should we use gsl::Pointer in the comments too? Or do you have an alternative in mind?


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:16
+
+struct [[gsl::Owner(int)]] T {
+  T();
----------------
gribozavr wrote:
> Can `T` and `MyOwner` be the same type?
> 
> It is confusing to have two -- for example, `toOwner()` returning `T` instead of `MyOwner` is confusing.
I agree that T might not be a great name. The reason why we have two types because one can be converted to a Pointer using a conversion operator the other can be converted using a one argument non-explicit constructor. These two are generating different ASTs, thus I wanted to test both ways. 


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:33
+  MyPointer p{&i}; // ok
+  new MyPointer(MyPointer{p}); // ok
+}
----------------
gribozavr wrote:
> Why is the last line OK? Looks like a false negative to me -- the pointer on the heap is pointing to a local variable, just like in `f`, which produces a warning.
> 
> If it is an intentional false negative, please annotate it as such in comments.
Yeah, this is an intentional false negative as we do not have enough information in a statement local analysis to warn about this case. Will fix the comment thanks!


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:38
+  T t;
+  return t.release(); // ok
+}
----------------
gribozavr wrote:
> Is "release" a magic name that the warning understands?
Method calls are not handled yet, but the idea is to understand some STL specific methods, like the `std::unique_ptr::release`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64256