Re: WIN32 pg_import_system_collations

On Mon, Oct 31, 2022 at 3:09 PM Peter Eisentraut <
peter(dot)eisentraut(at)enterprisedb(dot)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