On Wed, Sep 28, 2022 at 12:04 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Sep 28, 2022 at 2:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Peter Smith <smithpb2250@gmail.com> writes:
> > > Enums index a number of the GUC tables. This all relies on the
> > > elements being carefully arranged to be in the same order as those
> > > enums. There are comments to say what enum index belongs to each table
> > > element.
> > > But why not use designated initializers to enforce what the comments
> > > are hoping for?
> >
> > Interesting proposal, but it's not clear to me that this solution makes
> > the code more bulletproof rather than less so. Yeah, you removed the
> > order dependency, but the other concern here is that the array gets
> > updated at all when adding a new enum entry. This method seems like
> > it'd help disguise such an oversight. In particular, the adjacent
> > StaticAssertDecls about the array lengths are testing something different
> > than they used to, and I fear they lost some of their power.
>
> Thanks for the feedback!
>
> The current code StaticAssertDecl asserts that the array length is the
> same as the number of enums by using hardwired knowledge of what enum
> is the "last" one (e.g. DEVELOPER_OPTIONS in the example below).
>
> StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 2),
> "array length mismatch");
>
> Hmmm. I think maybe I found the example to justify your fear. It's a
> bit subtle and AFAIK the HEAD code would not suffer this problem ---
> imagine if the developer adds the new enum before the "last" one (e.g.
> ADD_NEW_BEFORE_LAST comes before DEVELOPER_OPTIONS) and at the same
> time they *forgot* to update the table elements, then that designated
> index [DEVELOPER_OPTIONS] will still ensure the table becomes the
> correct increased length (so the StaticAssertDecl will be ok) except
> now there will be an undetected "hole" in the table at the forgotten
> [ADD_NEW_BEFORE_LAST] index.
>
> > Can we
> > improve those checks so they'd catch a missing entry again?
>
> Thinking...
>
Although adding designated initializers did fix some behaviour of the
current code (e.g. which assumed array element order, but cannot
enforce it), it also introduces some new unwanted quirks (e.g.
accidentally omitting table elements can now be undetectable).
I can't see any good coming from exchanging one kind of problem for a
new kind of problem, so I am abandoning the idea of using designated
initializers in these GUC tables.
~
The v2 patches are updated as follows:
0001 - Now this patch only fixes a comment that had a wrong enum name.
0002 - Removes unnecessary whitespace (same as v1-0002)
------
Kind Regards,
Peter Smith.
Fujitsu Australia