-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[libcxx] Provide locale conversions to tests through lit substitution #105651
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the test suite doesn't always provide the thousands separator? If we're going through the trouble of figuring those out, wouldn't it make sense to always provide the define so the code doesn't have to use
thousands_sep_or_default
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, that rather than the individual tests doing
const wchar_t fr_sep = LocaleHelpers::mon_thousands_sep_or_default(FR_MON_THOU_SEP);
, we'd just goconst wchar_t fr_sep = FR_MON_THOU_SEP;
?I'm not entirely sure of the original author's intentions here, but I would guess that this felt like a smaller step - in case we didn't manage to dig up the right separators in the test framework. But as long as that setup does work (and afaik it does), I guess we could get rid of the defaults in
locale_helpers.h
entirely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work quite off the bat, at least.
The problem is that
FR_MON_THOU_SEP
is a wchar_t string likeL"\u00a0"
, while we want a singlewchar_t
. Themon_thousands_sep_or_default
helper took astd::wstring
and returnss[0]
ifs
is nonempty in the current version of the patch.This is because what
localeconv()
returns is a struct with strings, where each separator string usually is one single char, but they could in theory be multiple. In the current patch we treat this as strings all the way up to thefoo_sep_or_default()
helpers which convert from a string to a single wchar.If we wanted to go that way, I guess we could make the
localeconv()
helper executable, that is called in python, only return the first char, and treat it as a numeric single wchar throughout instead. I guess that'd work too. It would be a bit tricky for one of the tests, though, where we currently try to do this:I'm not sure how we'd easily synthesize a wchar string literal out of that, when we'd have e.g.
-DFR_THOU_SEP=0x00a0
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mstorsjo for taking over the PR and making progress on it!
Right, localeconv's struct has
currency_symbol
which could in practice be more than 1 char, so to allowprovide_locale_conversions
substitution code in features.py to be flexible enough it just sets the substitutions as strings.The defaults ("," and ".") in the
thousands_sep_or_default
helper function won't actually be used as we are only calling these from tests where the locale is set to fr_FR or ru_RU and we've set the defines.I think it's reasonable for these helpers to assert that the input is not empty and always take the first char. The functions should probably be renamed to something more appropriate though if they're essentially just converting to a wchar_t.