On Tue, 2025-11-25 at 09:44 +0800, Chao Li wrote:
> I was curious why “inline” is needed, then I figured out when I tried
> to build. Without “inline”, compile will raise warnings of “unused
> function”. So I guess it’s better to explain why “inline” is used in
> the function comment, otherwise other readers may get the same
> confusion.
That's a typical pattern: to make it "inline", move it to a .h file and
declare it as "static inline". For common patterns like that, I don't
think we should explain them in comments, because it would mean we
would start adding comments in zillions of places.
> With “change in length”, I confirmed “Unicode 5.18.2” means the
> Unicode Standard Section 5.18.2 “Complications for Case Mapping”. Why
> don’t we just give the URL in the comment.
> https://www.unicode.org/versions/Unicode17.0.0/core-spec/chapter-5/#G29675
Done, thank you. (Though since we haven't actually moved to 17 yet, I
linked to 16 instead.)
> I don’t get this change. In old code, depending on locale->ctype-
> >strfold, it calls strfold or strlower. But in this patch, it only
> calls strfold. Why? If that’s intentional, maybe better to add a
> comment to explain that.
I thought it would be slightly cleaner to just define the strfold
method in the libc provider as the same as strlower. I agree it's worth
a comment, so I added some in pg_locale_libc.c.
> In pg_strfold, the ctype==NULL fallback code is exactly the same as
> pg_strlower. I guess you intentionally to not call pg_strlower here
> for performance consideration, is that true?
I made some static functions to clean that up, and added some comments.
Thank you.
New series attached with only these changes and a rebase.
Regards,
Jeff Davis
