NoQ added a comment.
In D107078#2927899 <https://reviews.llvm.org/D107078#2927899>, @steakhal wrote:
> I did not fix the extra blue 'bubble' note issue, and I think that is out of
> the scope of this patch.
This is an on-by-default checker. We should not knowingly regress it, even if
temporarily.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:397-398
+ Ctx.getLocationContext());
+ Report->addNote("The temporary object gets destroyed at the end of the "
+ "full expression",
+ L);
----------------
steakhal wrote:
> NoQ wrote:
> > This is part of the path, right? We're reporting these bugs at
> > `checkEndFunction` rather than at the store site so the destruction has
> > already happened by the time we report it. In this case it should be a path
> > note, not an extra blue-bubble note.
> >
> > As for wording, I suggest `full-expression` with a dash (that's what other
> > clang diagnostics use) and probably drop the initial "The"(?)
> > This is part of the path, right? We're reporting these bugs at
> > `checkEndFunction` rather than at the store site so the destruction has
> > already happened by the time we report it. In this case it should be a path
> > note, not an extra blue-bubble note.
>
> I'm not exactly sure how to address this. AFAIK there is no way currently
> detecting the completion of a full-expression.
> I was thinking of `Check::RegionChanges`, `Check::PostStmt<Expr>`,
> `Check::Live`, but none of these fits my needs. Do you have anything specific
> in mind that could help me?
>
> Aside from that, the report looks reasonable even with a 'blue' bubble:
> (scan-build): {F18363520}
> (CodeChecker): {F18364795}
>
> ---
>
> > As for wording, I suggest full-expression with a dash (that's what other
> > clang diagnostics use) and probably drop the initial "The"(?)
> I see, thanks.
Wait, no, in your example the end of the full-expression is not part of the
path; it happens after the warning is emitted.
If it was, the right thing to do would have been to catch the destruction of
the object of interest rather than the end of the full-expression as such.
But in this case the note seems to be redundant. Lifetime of the temporary
object is fairly irrelevant to the report. It only matters that such lifetime
is longer than that of the stack address. But the note doesn't tell the user
how //long// that lifetime is; it only says how //short// it is. So i think
this note is redundant.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107078/new/
https://reviews.llvm.org/D107078
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
