[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:6510
     LifetimeBoundCall,
+    LifetimePointerInit,
+    LifetimeTempOwner
----------------
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 .


================
Comment at: clang/lib/Sema/SemaInit.cpp:6587
+
 static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
                                         LocalVisitor Visit) {
----------------
I feel like the code would be better off if we defined another function for handling gsl::Pointer semantics.

This function is for clang::lifetimebound, and mixing the logic is confusing IMO.


================
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
----------------
Sorry, I can't parse this: "... initializing lifetime Pointer ..."


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:10
+struct [[gsl::Pointer(int)]] MyPointer {
+  MyPointer(int *p = 0);
+  MyPointer(const MyOwner &);
----------------
`= nullptr`


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:16
+
+struct [[gsl::Owner(int)]] T {
+  T();
----------------
Can `T` and `MyOwner` be the same type?

It is confusing to have two -- for example, `toOwner()` returning `T` instead of `MyOwner` is confusing.


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:20
+  int &operator*();
+  MyPointer release();
+  int *release2();
----------------
"ReleaseAsMyPointer'


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:21
+  MyPointer release();
+  int *release2();
+  int *c_str() const;
----------------
"ReleaseAsRawPointer"


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:22
+  int *release2();
+  int *c_str() const;
+};
----------------
This method is confusing -- is it a name that the warning is supposed to know about?


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:25
+
+void f() {
+  new MyPointer(T{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
----------------
It would be helpful to rename this test function and other functions below according to what they test.

`f`, `g`, `g2`, `i`, `i2` -- unclear what the test is supposed to be about, and why tests are grouped like that.


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:33
+  MyPointer p{&i}; // ok
+  new MyPointer(MyPointer{p}); // ok
+}
----------------
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.


================
Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:38
+  T t;
+  return t.release(); // ok
+}
----------------
Is "release" a magic name that the warning understands?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64256