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...
------
Kind Regards,
Peter Smith.
Fujitsu Australia
