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; } }