Re: pgsql: Trivial dead code removal: in makeObjectName(),

Log Message:
-----------
Trivial dead code removal: in makeObjectName(), name1 must be non-NULL
(due to the preceding strlen(), for example), so we needn't recheck this
before invoking pg_mbcliplen().

Per Coverity static analysis performed by EnterpriseDB.

Modified Files:
--------------
pgsql/src/backend/commands:
indexcmds.c (r1.131 -> r1.132)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/indexcmds.c.diff?r1=1.131&r2=1.132)

neilc(at)svr1(dot)postgresql(dot)org (Neil Conway) writes:
> Trivial dead code removal: in makeObjectName(), name1 must be non-NULL
> (due to the preceding strlen(), for example), so we needn't recheck this
> before invoking pg_mbcliplen().

I don't like either this change or the tab-complete one: this seems to
me to reduce readability, as well as robustness in the face of future
changes, in order to save nothing at all worth mentioning. If these
were performance-critical paths it might be worth doing, but they are
not. What's more, given a reasonably-intelligent optimizing compiler
the same deductions would be made at compile time, and so it's a fair
bet that you have bought not roughly but exactly zero in return for
making the code more fragile and less readable.

The schemacmds.c change seems OK since it's not introducing any obvious
path for future breakage.

regards, tom lane

Tom Lane wrote:
> I don't like either this change or the tab-complete one: this seems to
> me to reduce readability, as well as robustness in the face of future
> changes, in order to save nothing at all worth mentioning.

Can you elaborate on why you feel this reduces readability? I think it
is a slight readability improvement: keeping the redundant "if" check
suggests that the code is prepared to handle a NULL `name1', whereas
that is not the case. By removing the "if", it makes the code's
assumption (that name1 is non-NULL) more clear. A similar line of
reasoning applies to the tab-complete change.

As for "robustness in the face of future change", I don't see how it
makes any difference. The code presently assumes that `name1' is
non-NULL. If that changes, references to `name1' will need to be
checked, but that is neither difficult nor unexpected if you change the
range of values that can be assigned to `name1'. Or would you have us
add redundant "if (var)" checks before using any pointer variable, on
the principle that at some point in the future the code might change so
that the variable could be NULL?

-Neil