| Message ID | pull.1401.git.git.1671459163559.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove redundant double exclamation points | expand |
On Mon, Dec 19 2022, Rose via GitGitGadget wrote: > From: Seija Kijin <doremylover123@gmail.com> > > S_ISDIR is a macro that involves a "==" comparison. It does? The POSIX standard (https://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/stat.h.html) says: The following macros shall be provided to test whether a file is of the specified type. The value m supplied to the macros is the value of st_mode from a stat structure. The macro shall evaluate to a non-zero value if the test is true; 0 if the test is false. The "non-zero" there seems to intentionally leave open that this may be defined e.g. as via a "&" test, as opposed to "==" which according to C99's 6.5.9.3 says: The == (equal to) and != (not equal to) operators are analogous to the relational operators except for their lower precedence.90) Each of the operators yields 1 if the specified relation is true and 0 if it is false. The result has type int. For any pair of operands, exactly one of the relations is true. > This means the !! is redundant and not needed. I think you're therefore introducing a bug here, this may work on your platform, but we have no guarantee that it'll work elsewhere. I thought that it probably wouldn't matter, as we'd treat the argument as a boolean, but we don't. In within_depth() we proceed to use the passed-in 3rd argument (depth) as a counter, so it really does matter if it's 0, 1, or other non-zero. > tree-walk.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/tree-walk.c b/tree-walk.c > index 74f4d710e8f..6b51d27ccb2 100644 > --- a/tree-walk.c > +++ b/tree-walk.c > @@ -1040,9 +1040,9 @@ static enum interesting do_match(struct index_state *istate, > ps->max_depth == -1) > return all_entries_interesting; > return within_depth(base->buf + base_offset, baselen, > - !!S_ISDIR(entry->mode), > - ps->max_depth) ? > - entry_interesting : entry_not_interesting; > + S_ISDIR(entry->mode), ps->max_depth) ? > + entry_interesting : > + entry_not_interesting; > } > > pathlen = tree_entry_len(entry); Aside from whether or not this is a bug, could you please submit proposed refactorings of the git project via coccinelle patches if possible (as I suggested to you before). I realize that it has a slight learning curve, but it makes writing & maintaining these so much easier, and it'll fix (mis)uses going forward, not just as a one-off. So, as an example (and assuming this wasn't buggy), you'd do that in this case as e.g. (untested, but you can see similar syntax in our existing *.cocci files): @@ @@ - !! ( S_ISDIR | S_ISFIFO // | // we'd continue to list the rest here... ) (...)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I think you're therefore introducing a bug here, this may work on your > platform, but we have no guarantee that it'll work elsewhere. Thanks, well analysed.
diff --git a/tree-walk.c b/tree-walk.c index 74f4d710e8f..6b51d27ccb2 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -1040,9 +1040,9 @@ static enum interesting do_match(struct index_state *istate, ps->max_depth == -1) return all_entries_interesting; return within_depth(base->buf + base_offset, baselen, - !!S_ISDIR(entry->mode), - ps->max_depth) ? - entry_interesting : entry_not_interesting; + S_ISDIR(entry->mode), ps->max_depth) ? + entry_interesting : + entry_not_interesting; } pathlen = tree_entry_len(entry); @@ -1073,8 +1073,7 @@ static enum interesting do_match(struct index_state *istate, if (within_depth(base_str + matchlen + 1, baselen - matchlen - 1, - !!S_ISDIR(entry->mode), - ps->max_depth)) + S_ISDIR(entry->mode), ps->max_depth)) goto interesting; else return entry_not_interesting;