NoQ added inline comments.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:396
+ ProgramStateRef State, const LocationContext *FromLC,
+ const LocationContext *ToLC) {
+ const LocationContext *LC = FromLC;
----------------
a.sidorin wrote:
> EndLC? (similar to iterators)
Sounds neat, i'd make another quick patch on all things together.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+
+ if (!TR) {
+ StorageDuration SD = MT->getStorageDuration();
----------------
dcoughlin wrote:
> Would it be safe for the body of the `if (!TR)` to be the else branch of `if
> constCXXTempObjectionRegion *const *TRPtr = ...` rather then its own if
> statement?
Yep. Fxd.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:289
}
if (!TR)
TR = MRMgr.getCXXTempObjectRegion(Init, LC);
----------------
dcoughlin wrote:
> Would it be safe for `TR = MRMgr.getCXXTempObjectRegion(Init, LC);` to be the
> else branch of `if (const MaterializeTemporaryExpr *MT =
> dyn_cast<MaterializeTemporaryExpr>(Result))` rather than its own `if`
> statement?
>
> Ideally the number paths through this function on which we call
> `MRMgr.getCXXTempObjectRegion()` would be small and very clear.
>
Nope, it also covers the case when we have an MTE but it's not static. I guess
it's still more clear to add it twice than to have it as a fallback for an
indefinite amount of cases.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:490
+static void printTemporaryMaterializatinosForContext(
+ raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep,
----------------
dcoughlin wrote:
> Nit: There is a typo in the name here ("Materializatinos"). I guess this is
> the sub-atomic particle created as a byproduct of materializing a temporary!
> We have a lot of materializatinos in Cupertino.
Fixed to prevent further lifetime extensino of typos through autocompletinos.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:503
+ Key.first->printPretty(Out, nullptr, PP);
+ if (Value)
+ Out << " : " << Value;
----------------
a.sidorin wrote:
> As I see from line 350, `Value` is always non-null. And there is same check
> on line 659. Am I missing something?
Yep, all of these are non-null by construction. This actually makes me wonder
if it was correct to remove `VisitCXXBindTemporaryExpr` from `ExprEngine` -
might have done it too early (this is what makes the similar if in
`printInitializedTemporariesForContext` redundant). The
`dont_forget_destructor_around_logical_op` test also suggests that - seems to
actually be a regression. I guess i'm going to revert the
`VisitCXXBindTemporaryExpr` change in a follow-up commit.
Also we should allow `ostream << MR` to accept null pointers. It's a global
operator overload anyway.
================
Comment at: test/Analysis/lifetime-extension.cpp:45
+ clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}
----------------
dcoughlin wrote:
> Yay!
These worked before (with old lifetime extension, since temporary constructors
were inlined), i only added the run-line to see that.
https://reviews.llvm.org/D43497
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
