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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6549
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef<IndirectLocalPathEntry> Path) {
+  return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {
----------------
Move closer to the point of usage?


================
Comment at: clang/lib/Sema/SemaInit.cpp:6549
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef<IndirectLocalPathEntry> Path) {
+  return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {
----------------
gribozavr wrote:
> Move closer to the point of usage?
re: name, `pathOnlyContainsGslPointerInit` or `pathOnlyInitializesGslPointer`


================
Comment at: clang/lib/Sema/SemaInit.cpp:6564
+template<typename T>
+static bool recordHasAttr(QualType Type) {
+  if (auto *RD = Type->getAsCXXRecordDecl())
----------------
"isRecordWithAttr" ?


================
Comment at: clang/lib/Sema/SemaInit.cpp:7072
+
+    bool IsLifetimePtrInitWithTempOwner = false;
+    if (!Path.empty() &&
----------------
Please adjust the name.


================
Comment at: clang/lib/Sema/SemaInit.cpp:7076
+        auto Prefix = llvm::makeArrayRef(Path).drop_back();
+      if (pathInitializeLifetimePointer(Prefix))
+        IsLifetimePtrInitWithTempOwner = true;
----------------
Is it important that the whole path only contains gsl::Pointer nodes?


================
Comment at: clang/lib/Sema/SemaInit.cpp:6510
     LifetimeBoundCall,
+    LifetimePointerInit,
+    LifetimeTempOwner
----------------
xazax.hun wrote:
> 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?
My suggestion is to refer to a type that was annotated with gsl::Pointer as a "gsl::Pointer type" in prose, or "GslPointer" where you need an identifier.

So I agree about `GslPointerInit`.

As for the second one, it seems like `GslOwnerTemporaryInit` would be more fitting -- WDYT? It is an initialization of a temporary that has a gsl::Owner type.


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:16
+
+struct [[gsl::Owner(int)]] T {
+  T();
----------------
xazax.hun wrote:
> 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. 
Please explain this distinction in the comments in the test. If you didn't tell me just now, I would never know.

Maybe it is also a good idea to split the MyPointer type then, let each owner have its own pointer. Right now having one pointer with two owners is confusing, it suggests that owners are somehow related, but they aren't.


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

https://reviews.llvm.org/D64256