On Tue, Dec 11, 2018 at 11:23:13AM +0100, Michal Hocko wrote: >On Tue 11-12-18 10:19:05, Wei Yang wrote: >> On Tue, Dec 11, 2018 at 10:44:41AM +0100, Michal Hocko wrote: >> >On Tue 11-12-18 11:51:28, Wei Yang wrote: >> >> A valid present section number is in [0, __highest_present_section_nr]. >> >> And the return value of next_present_section_nr() meets this >> >> requirement. This means it is not necessary to check it with >> >> __highest_present_section_nr again in for_each_present_section_nr(). >> >> >> >> Since we pass an unsigned long *section_nr* to >> >> for_each_present_section_nr(), we need to cast it to int before >> >> comparing. >> > >> >Why do we want this patch? Is it an improvement? If yes, it is >> >performance visible change or does it make the code easier to maintain? >> > >> >> Michal >> >> I know you concern, maintainance is a very critical part of review. >> >> >To me at least the later seems dubious to be honest because it adds a >> >non-obvious dependency of the terminal condition to the >> >next_present_section_nr implementation and that might turn out error >> >prone. >> > >> >> While I think the original code is not that clear about the syntax. >> >> When we look at the next_present_section_nr(section_nr), the return >> value falls into two categories: >> >> -1 : no more present section after section_nr >> other: the next present section number after section_nr >> >> Based on this syntax, the iteration could be simpler to terminate >> when the return value is less than 0. This is what the patch tries to >> do. >> >> Maybe I could do more to help the maintainance: >> >> * add some comment about the return value of next_present_section_nr >> * terminate the loop when section_nr == -1 >> >> Hope this would help a little. > >Well, not really. Nothing of the above seems to matter to callers of the >code. So I do not see this as a general improvement and as such no >strong reason to merge it. It is basicly polishing a code without any >obvious issues. Er... but I don't see the reason to keep a redundant check in the code. Even this is an internal function, it would be better to make it clean and neat. Would you mind sharing your concern about this polishing? If there is no issue, we would prefer no polishing of the code? >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me
