Re: Improve readability by using designated initializers when possible

On Mon, Mar 04, 2024 at 09:29:03AM +0800, jian he wrote:
> On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>> Oops, there was a second commit in my branch that I neglected to send
>> in. Here is my complete patch set.

Thanks for the new patch set. The gains are neat, giving nice
numbers:
7 files changed, 200 insertions(+), 644 deletions(-)

+ default:
+ DropObjectById(object);
+ break;

Hmm. I am not sure that this is a good idea. Wouldn't it be safer to
use as default path something that generates an ERROR so as this code
path would complain immediately when adding a new catalog? My point
is to make people consider what they should do on deletion when adding
a catalog that would take this code path, rather than assuming that a
deletion is OK to happen. So I would recommend to keep the list of
catalog OIDs for the DropObjectById case, keep the list for global
objects, and add a third path with a new ERROR.

- /*
- * There's intentionally no default: case here; we want the
- * compiler to warn if a new OCLASS hasn't been handled above.
- */

In getObjectDescription() and getObjectTypeDescription() this was a
safeguard, but we don't have that anymore. So it seems to me that
this should be replaced with a default with elog(ERROR)?

There is a third one in getObjectIdentityParts() that has not been
removed, though, but same remark at the two others.

RememberAllDependentForRebuilding() uses a default, so this one looks
good to me.

> there is a `OCLASS` at the end of getObjectIdentityParts.

Nice catch. A comment is not updated.

> There is a `ObjectClass` in typedefs.list

This is usually taken care of by committers or updated automatically.
--
Michael