Thread: pgsql: Trivial dead code removal: in makeObjectName(), name1 must be
pgsql: Trivial dead code removal: in makeObjectName(), name1 must be
From
neilc@svr1.postgresql.org (Neil Conway)
Date:
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@svr1.postgresql.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