Stefan Beller <sbeller@xxxxxxxxxx> writes:
> Maybe I have read too much of the refs code lately as there we
> have these long chains which combine precondition with error
> checking.
Of course, things are not so black-and-white in the real world.
You can also take an extreme stance and view
if (!pretend && do_it_and_report_error())
error(...);
differently. I would explain that what the whole thing is trying to
achieve as "'do it' part is the primary thing we want to do, but it
only can be done when we are not pretending, and we show an error
when the 'do it' part fails." and would suggest structuring it more
like this:
if (pretend)
; /* nothing */
else if (do_it_and_report_error())
error(...);
or
if (!pretend) {
if (do_it_and_report_error())
error(...);
}
But you could say "The final objective of the whole thing is to show
an error message, but if we are pretending or if our attempt to 'do
it' succeeds, we do not have to show the error", and such a view may
make sense depending on what that 'do it' is. The original may be
justified under such an interpretation.
We may be tempted to write (note: each boolean term may be a call
with many complex arguments)
if (A && B && C && D && E)
...
when it is in fact logically is more like this:
/* does not make sense to attempt C when A && B does not hold */
if (A && B) {
if (C && D && E)
...
}
But it becomes a judgement call if splitting that into nested two if
statements is better for overall readability when the top-level if
statement has an else clause. You cannot turn it into
/* does not make sense to attempt C when A && B does not hold */
if (A && B) {
if (C && D && E)
...
} else {
... /* this cannot be what the original's else clause did */
}
blindly. It would need further restructuring. I think the code
inside the refs.c is a result of making such judgement calls.
>> By the way, aren't we leaking ce when we are merely pretending?
>
> Yes we are, that's how I found this spot. (coverity pointed out ce was
> leaking, so I was refactoring to actually make it easier to fix it, and then
> heavily reordered the patch series afterwards. That spot was forgotten
> apparently.
I dropped 2/15 and expect the real fix in the future; no rush,
though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html