On Thu, Mar 10, 2022 at 10:52:41AM +0100, Peter Eisentraut wrote:
> On 05.03.22 09:38, Julien Rouhaud wrote:
> > @@ -168,18 +175,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
> > errmsg("collation \"default\" cannot be copied")));
> > }
> >
> > - if (localeEl)
> > - {
> > - collcollate = defGetString(localeEl);
> > - collctype = defGetString(localeEl);
> > - }
> > [...]
> >
> > I tried to read the function and quickly got confused about whether possible
> > problematic conditions could be reached or not and had protection or not. I
> > think that DefineCollation is becoming more and more unreadable, with a mix of
> > fromEl, !fromEl and either. Maybe it would be a good time to refactor the code
> > a bit and have have something like
> >
> > if (fromEl)
> > {
> > // initialize all specific things
> > }
> > else
> > {
> > // initialize all specific things
> > }
> >
> > // handle defaults and sanity checks
> >
> > What do you think?
>
> How about the attached?
Thanks! That's exactly what I had in mind. I think it's easier to follow and
make sure of what it's doing exactly, so +1 for it.
