On Mon, Oct 31, 2022 at 3:09 PM Peter Eisentraut < peter.eisentr...@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
v7-0002-Add-collate.windows.win1252-test.patch
Description: Binary data
v7-0001-WIN32-pg_import_system_collations.patch
Description: Binary data
