[PATCH] D16403: Add scope information to CFG

Maxim Ostapenko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 10 03:15:58 PDT 2017


m.ostapenko added a comment.

Hi Artem, I'm sorry for a long delay (nasty corporate issues).

In https://reviews.llvm.org/D16403#789957, @NoQ wrote:

> Maxim, totally thanks for picking this up!
>
> Could you explain the idea behind `shouldDeferScopeEnd`, maybe in a code comment before the function?
>
> In https://reviews.llvm.org/D16403#788926, @m.ostapenko wrote:
>
> > Current patch should support basic {If, While, For, Compound, Switch}Stmts as well as their interactions with {Break, Continue, Return}Stmts.
> >  GotoStmt and CXXForRangeStmt are not supported at this moment.
>
>
> `SwitchStmt` isn't much easier than `GotoStmt`; it doesn't jump backwards, but it can still jump into multiple different scopes. Does your code handle Duff's device (1) <https://www.lysator.liu.se/c/duffs-device.html> (2) <https://en.wikipedia.org/wiki/Duff's_device> correctly? We should probably add it as a test, or split out switch support into a separate patch together with goto, if such test isn't yet supported.


Ugh, yeah, SwitchStmt is tricky (thank you for pointing to Duff's device example!). I've tried to resolve several issues with switch revealed by this testcase, but didn't succeed for now :(. So, it was decided to remove SwitchStmt support in this patch.
There is one more thing I'd like to clarify: since e.g. BreakStmt can terminate multiple scopes, do I need to create ScopeEnd marks for all of them to make analyzer's work easier? Because right now only one "cumulative" block is generated and I'm not sure that it's acceptable for analyzer.


Repository:
  rL LLVM

https://reviews.llvm.org/D16403





More information about the cfe-commits mailing list