Ranier Vilela <ranier.vf@gmail.com> writes:
> Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
> file, to correct it, a simplification was made and the oom_error goto was
> removed, since it is clearly redundant and its presence can be confusing.
I'm kind of disinclined to let Coverity dictate our coding style here.
We've dismissed many hundreds of its reports as false positives, and
this seems like one that could get (probably already has gotten) the
same treatment. I also don't feel like duplicating error messages
as you propose is an improvement.
If you look closely at the code in the source, you will see that the style there is:
if (test)
{
msg;
goto;
}
I just kept it, even if I duplicated the error message, the style was kept and in my opinion it is much more coherent and readable.
But your solution is also good, and yes, it is worth it, because even with small benefits, the change improves the code and prevents Coverity or another tool from continuing to report false positives or not.
If we did want to adjust the code in pg_SASL_init, my thought would
be to reduce not increase the code duplication, by making the error
exits look like
...
return STATUS_OK;
oom_error:
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("out of memory\n"));
/* FALL THRU */
error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
return STATUS_ERROR;
}
It's only marginally worth the trouble though.
Sounds good to me.
> First, a correction was made to the return types of some functions that
> clearly return bool, but are defined as int.
This is ancient history that doesn't seem worth revisiting. There is
certainly exactly zero chance of us changing libpq's external API
as you propose, because of the ensuing ABI breakage. Maybe we could
change the static functions, but I'm not very excited about it.
Virtually no code will break for the change, since bool and int are internally the same types.
I believe that no code will have either adjusted to work with corrected functions, even if they use compiled libraries.
And again, it is worth correcting at least the static ones, because the goal here, too, is to improve readability.
I can't get excited about the other code rearrangements you're proposing
here either. They seem to make the code more intellectually complex for
little benefit.
I cannot agree with you that these changes add complexity.
It was using the principle enshrined in programming that I proposed these changes.
"Get out quick."
For 99% of calls to these functions, there won't be any changes, since all parameters are ok, tests will be done and the PQsendQueryStart function will be called anyway.
If it were possible, it would be better to eliminate the basic tests, but this is not possible, so better to do them first and get out of there soon, without doing anything else.
regards,
Ranier Villela
