Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable glibc undocumented querylocale() #22701

Open
wants to merge 1 commit into
base: maint-5.38
Choose a base branch
from

Conversation

khwilliamson
Copy link
Contributor

@khwilliamson khwilliamson commented Oct 25, 2024

This is for maint 5.38. This code is already effectively in 5.40.

This is querylocale() by another name. This is required because of GH #21366.

In order to maintain the ABI with previous releases of 5.38, the two symbols PL_curlocales and PL_cur_LC_ALL are now always compiled. This keeps the structure size the same as before this commit, with the space these occupy now unused.

  • This set of changes requires a perldelta entry, and I need help writing it.

This is querylocale() by another name.  This is required because of
GH Perl#21366.

In order to maintain the ABI with previous releases of 5.38, the two
symbols PL_curlocales and PL_cur_LC_ALL are now always compiled.  This
keeps the structure size the same as before this commit, with the space
these occupy now unused.
Comment on lines -751 to -762
#ifdef USE_PL_CURLOCALES

/* This is the most number of categories we've encountered so far on any
* platform, doesn't include LC_ALL */
PERLVARA(I, curlocales, 12, const char *)

#endif
#ifdef USE_PL_CUR_LC_ALL

PERLVARI(I, cur_LC_ALL, const char *, NULL)

#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this change the layout for a perl built with -DNO_LOCALE?

Even with that fixed I'm hesitant to backport this change - I'm not sure it qualifies under the conditions from perlpolicy, and it appears to be a minor enhancement at best.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#21366, which this fixes, is a regression introduced in 5.38. Why would that not be something we need to fix? I don't understand about this being an "enhancement". It is a bare minimum fix.

I believe you are right about NO_LOCALE, and it is fixable as you surmised. I would want to make sure there is no other combination that would need to be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have followed through the reference you provided.

I think it's reasonable to apply this is the bincompat is fixed.

I do worry about systems that don't have a querylocale() work-a-like (are there any? musl provides this extension) -- if I understand the issue this won't fix such systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants