A condition could be a multi-line expression where we create the highlight
in separated chunks. PathDiagnosticPopUpPiece is not made for that purpose,
it cannot be added to multiple lines because we have only one ending part
which contains all the notes. So that it cannot have multiple endings and
therefore this patch narrows down the ranges of the highlight to the given
interesting variable of the condition. It prevents HTML-breaking injections.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
It also reverts https://reviews.llvm.org/rL362632 which is not a fix, but a problem instead.
Aha, aha, looks reasonable. If we're describing a variable, we should only highlight that variable.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | ||
---|---|---|
2420 ↗ | (On Diff #213063) | Just curious, can BExpr->getLHS() potentially still be a multi-line expression? Or are we making sure it's always a DeclRefExpr/MemberExpr? In case of MemberExpr i'm pretty sure you can fit a newline before/after . or ->. |
2512 ↗ | (On Diff #213063) | It looks like you forgot to make this change popup-piece-specific. I think we should add our note to the whole condition in case of event pieces. |
Here is an example of the new MemberExpr::getBase() based report:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | ||
---|---|---|
2420 ↗ | (On Diff #213063) | Previously I have focused on DeclRefExpr and MemberExpr value-evaluation, so that there the expression must be one of them. Because we have no getField() method for obtaining the field of the MemberExpr, I have picked getBase(). |
2512 ↗ | (On Diff #213063) | Hm, it probably makes sense. At this level 'Cond' and 'ME' are equals as I saw, but may in crazy conditions they are not. |
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | ||
---|---|---|
2420 ↗ | (On Diff #213063) | It's MemberExpr::getMemberLoc(). It's always a single token. Unlike the base, which may still be arbitrarily complex. |
Thanks for the review! It is much better:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | ||
---|---|---|
2420 ↗ | (On Diff #213063) | I am thinking about the opposite when we have->a->lot->of->stuff. It is working fine, thanks for the ideas! |