NoQ added inline comments.
================
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
----------------
Tyker wrote:
> riccibruno wrote:
> > I don't understand why this is needed. Can you explain it ? Also I think
> > that someone familiar with this code should comment on this (maybe @NoQ ?)
> the detail of why are complicated and i don't have them all in head but
> without this edit in cases like
>
> ```
> switch (...) {
> [[likely]] case 1:
> ...
> [[fallthrough]];
> default:
> ...
> }
> ```
> the fallthrough attribute emitted a diagnostic because is wasn't handling
> attributed case statement. the edit i performed is probably not the optimal
> way to solve the issue as it only solves the issue for likelihood attribute.
> but i don't know any other attribute that can be applied on a case statement
> but if they were others they would probably have the same issue. but the code
> is quite hard to follow and i didn't wanted to break anything. so this is
> what i came up with.
> i am going to look into it to find a better solution.
The [[likely]] attribute should not affect the overall topology of the CFG. It
might be a nice piece of metadata to add to a CFG edge or to a CFG terminator,
but for most consumers of the CFG (various static analyses such as
analysis-based warnings or the Static Analyzer) the attribute should have
little to no effect - the tool would still need to explore both branches. I
don't know how exactly the fallthrough warning operates, but i find it likely
(no pun intended) that the fallthrough warning itself should be updated, not
the CFG.
It is probable that for compiler warnings it'll only cause false negatives,
which is not as bad as false positives, but i wouldn't rely on that.
Additionally, false negatives in such rare scenarios will be very hard to
notice later. So i'm highly in favor of aiming for the correct solution in this
patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
