On Sat, Jul 03 2021, ZheNing Hu wrote:
> Christian Couder <christian.couder@xxxxxxxxx> 于2021年7月3日周六 上午6:11写道：
>>
>> On Fri, Jul 2, 2021 at 9:28 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> >
>> > Aside from the potential maintenance burden of the `atom_type >= ...
>> > && atom_type <= ...` approach, another problem is that it increases
>> > cognitive load. The reader has to go reference the enum to fully
>> > understand the cases to which this code applies. On the other hand,
>> > the way the patch mentions the enumeration items explicitly, it is
>> > obvious at-a-glance to which cases the code applies. An additional
>> > downside of the suggestion is that the range specified by `>=` and
>> > `<=` may cause some readers to think that there is some sort of
>> > implicit relationship between the items in the range, which doesn't
>> > seem to be the case. So, I find the way it's done in the patch
>> > presently easier to comprehend.
>>
>> I agree that it's less cognitive load, but maybe it could be improved
>> using a separate function like:
>>
>> static int reject_atom(int cat_file_mode, enum atom_type atom_type)
>> {
>> if (!cat_file_mode)
>> return atom_type == ATOM_REST;
>>
>> /* cat_file_mode */
>> switch (atom_type) {
>> case ATOM_FLAG:
>> case ATOM_HEAD:
>> case ATOM_PUSH:
>> case ATOM_REFNAME:
>> case ATOM_SYMREF:
>> case ATOM_UPSTREAM:
>> case ATOM_WORKTREEPATH:
>> return 1;
>> default:
>> return 0;
>> }
>> }
>
> I agree. It is clearer to use a function to wrap origin reject atoms step.
>
> Thanks.
Yeah I guess you're both right, I just found the dense giant if to be
hard to read, but having a helper is even better.
