On Thu, Feb 22, 2024 at 5:56 PM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Thu, Feb 22, 2024 at 05:30:08PM +1100, Peter Smith wrote:
> > Oops. Perhaps I meant more like below -- in any case, the point was
> > the same -- to ensure RS_INVAL_NONE is what returns if something
> > unexpected happens.
>
> You are right that this could be a bit confusing, even if we should
> never reach this state. How about avoiding to return the index of the
> loop as result, as of the attached? Would you find that cleaner?
> --
Hi, yes, it should never happen, but thanks for making the changes.
I would've just removed every local variable instead of adding more of
them. I also felt the iteration starting from RS_INVAL_NONE instead of
0 is asserting RS_INVAL_NONE must always be the first enum and can't
be rearranged. Probably it will never happen, but why require it?
------
ReplicationSlotInvalidationCause
GetSlotInvalidationCause(const char *conflict_reason)
{
for (ReplicationSlotInvalidationCause cause = 0; cause <=
RS_INVAL_MAX_CAUSES; cause++)
if (strcmp(SlotInvalidationCauses[cause], conflict_reason) == 0)
return cause;
Assert(0);
return RS_INVAL_NONE;
}
------
But maybe those nits are a matter of personal choice. Your patch code
addressed my main concern, so it LGTM.
----------
Kind Regards,
Peter Smith.
Fujitsu Australia
