Re: ICU for global collation

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.