On Tue, Mar 16, 2021 at 1:48 AM ZheNing Hu <adlternative@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 于2021年3月16日周二 上午7:41写道： > > > + strtol_i(reroll_count_string, 10, &reroll_count); > > > > This code can be confusing to readers since it appears to ignore the > > result of strtol_i() [...] > > [...] I think it would be clearer if you move the strtol_i() > > call into the diff_title() function since -- following your changes -- > > that function is the only code which cares whether `reroll_count` can > > be parsed as a number (the rest of the code, after your change, just > > uses it as a string). > > Well, The reason `strtol_i` does not check the return value is that > `strtol_i` will > only modify the value of `reroll_count` if the `reroll_count_string` > we provide is > an integer string, so if `reroll_count_string` is not an integer string, then > `reroll_count` will remain -1, and then `diff_title` will only execute > > if (reroll_count <= 0) > strbuf_addstr(sb, generic); > > So don't need check strtol_i return value. Yes, I understand the reason, but it is not easy for someone new to this code to figure it out without looking at diff_title() and its callers. The place where the return value of strtol_i() is ignored is very far removed from the place where the value of `reroll_count` is consumed in diff_title(), so it's not obvious to the reader how this is all supposed to work. If you move strtol_i() into diff_title(), then the parsing and the consuming of that value happen in the same place, making it easier to understand. > But what you said to put > `strtol_i` in > `diff_title` is indeed a good idea. Of course, we need to modify the declaration > and parameters of `diff_title`. Yes, it is a very minor modification to diff_title(), changing `reroll_count` from int to string. Moving the parsing into diff_title() also allows you to use the simpler name `reroll_count` for the string variable in cmd_fmt_patch() rather than the longer `reroll_count_string`; just change `reroll_count` from int to string.
