Re: [PATCH] libpq improvements and fixes

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 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.

> 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.

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.