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