[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

[Artem Dergachev via Phabricator via cfe-commits]

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