On Sun, Jul 09, 2017 at 07:44:46AM -0700, Christopher Li wrote: > On Sun, Jul 9, 2017 at 3:26 AM, Luc Van Oostenryck > <luc.vanoostenryck@xxxxxxxxx> wrote: > >> diff --git a/flow.c b/flow.c > >> index c7161d4..5d2f15a 100644 > >> --- a/flow.c > >> +++ b/flow.c > >> @@ -841,7 +841,7 @@ void kill_unreachable_bbs(struct entrypoint *ep) > >> } END_FOR_EACH_PTR(bb); > >> PACK_PTR_LIST(&ep->bbs); > >> > >> - repeat_phase &= ~REPEAT_CFG_CLEANUP; > >> + repeat_phase |= REPEAT_CSE ; > > > > It would be good to add a comment for why the '|= REPEAT_CSE' is needed here. > OK. I will update the patch. Basically removing unreachable dead code will > impact the us > > > > And not removing the REPEAT_CFG_CLEANUP is an error IMO. > > At the end of the function the CFG *is* clean. If you don't > > clear it here, then what is its meaning? > It is an error as concept or it is an error the program will do wrong > things? Concept. > I want to turn REPEAT_CFG_CLEANUP usage pattern similar > to REPEAT_SYMBOL_CLEANUP. > I just want to be consistent with other usage of the similar flags. For the other usage, the clearing of the flag (REPEAT_CSE) was done just after the CSE was done. You focus on the placement. I would focus on the meaning. But it's your choice. > I think when you introduce REPEAT_CFG_CLEANUP you have > different usage in mind. I found the flag clean at beginning of the > CSE loop is conceptually clean as well, it is simpler. The flag > is just a request, it can set more than one times. The action is > perform only once at one CSE pass. This doesn't sound technically convincing to me, not at all. Does the facts that: 1) it's a request flag 2) it can be set more than once imply in any sort of way that: 1) the action is to be performed at the CSE pass and only there 2) and thus the only sane place where the flag can be cleared is there like the other flags? By clearing the flag there and not at the end of the function doing the action, you're more or less forcing the action to be done there and only there. This may be a good enough reason to for whishing to enforce that but then tell it, put it in a nice comment in the code. This is certainly not "conceptually clean as well" to me. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
