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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
