Re: WIN32 pg_import_system_collations

On Mon, Oct 31, 2022 at 3:09 PM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

Thanks for taking a look into this patch.

>
> Consider this function you are introducing:
>
> +/*
> + * Create a collation if the input locale is valid for so.
> + * Also keeps track of the number of valid locales and collations created.
> + */
> +static int
> +CollationFromLocale(char *isolocale, char *localebuf, int *nvalid,
> +                                       int *ncreated, int nspid)
>
> This declaration is incomprehensible without studying all the callers
> and the surrounding code.
>
> Start with the name: What does "collation from locale" mean?  Does it
> make a collation?  Does it convert one?  Does it find one?  There should
> be a verb in there.
>
> (I think in the context of this file, a lower case name would be more
> appropriate for a static function.)
>
> Then the arguments.  The input arguments should be "const".  All the
> arguments should be documented.  What is "isolocale", what is
> "localebuf", how are they different?  What is being counted by "valid"
> (collatons?, locales?), and what makes a thing valid and invalid?  What
> is being "created"?  What is nspid?  What is the return value?
>
> Please make another pass over this.
>
> Ok, I can definitely improve the comments for that function.


> Also consider describing in the commit message what you are doing in
> more detail, including some of the things that have been discussed in
> this thread.
>
> Going through the thread for the commit message, I think that maybe the
collation naming remarks were not properly addressed. In the current
version the collations retain their native name, but an alias is created
for those with a shape that we can assume a POSIX equivalent exists.

Please find attached a new version.

Regards,

Juan José Santamaría Flecha