Taylor Blau <me@xxxxxxxxxxxx> writes:
>> + int has_positive = 0;
>> + int has_negative = 0;
>> +
>> + for (i = 0; (optch = optarg[i]) != '\0'; i++) {
>> + if (optch < 'a' || 'z' < optch)
>> + has_positive++;
>> + else
>> + has_negative++;
>> + }
>> +
>> + if (!has_positive && has_negative) {
>
> (One small nit that I absolutely do not care about is using a variable
> that starts with 'has_'--which I would expect to be a boolean--to count
> the number of positive/negative filters. Perhaps calling these
These variables are indeed used in the boolean sense (see how they
are used in the condition in the "if" statement).
If we wanted to short-circuit, we could do this:
for (i = 0;
!has_positive && !has_negative &&
(optch = optarg[i]) != '\0';
i++) {
if (isupper(optch))
has_negative++;
else
has_positive++;
}
and thanks to their names, nobody would be confused to find it is a
bug that these do not count when the loop exits early. We shouldn't
name these positive_nr or negative_nr because we are not interested
in counting them.
It is just that "bool++" is more idiomatic than repeated assignment
"bool = 1" when setter and getter both know it is merely used as a
Boolean, and that is why they named as has_X, which clearly is a
name for a Boolean, not a counter.
Having said that, I do not mind if an assignment is used instead of
post-increment in new code. I just think it is waste of time to go
find increments that toggle a Boolean to true and changing them to
assignments, and it is even more waste having to review such a code
churn, so let's not go there ;-)
But as I wrote in the big NEEDSWORK comment, this loop should
disappear when the code is properly rewritten to correct the
interaction between two or more "--diff-filter=" options, so it
would not matter too much.
Thanks.
