Details
- Reviewers
• Quuxplusone Mordante - Group Reviewers
Restricted Project - Commits
- rGd32f46b0766d: [libcxx] [test] Fix the get/put long_double_zh_CN tests on Windows
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/test/std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_zh_CN.pass.cpp | ||
---|---|---|
71–77 | Naming nit: By the time I saw it again below, I'd forgotten that curr_text stood for currency_name and not current_string. I suggest spelling out currency_{symbol,name}, or even better, refactoring into a helper function as you suggest in your PR summary. As in the previous PR, using a MultiStringType helper will eliminate the need for curr_x and w_curr_x to coexist. | |
libcxx/test/std/localization/locale.categories/category.monetary/locale.money.put/locale.money.put.members/put_long_double_zh_CN.pass.cpp | ||
82 | I suggest that this could be expressed as std::string(7 - curr_text.size(), ' ') and then inlined everywhere it's used. The name pad20 is awkward in that this string definitely isn't 20 bytes long. :) |
Applied most of the suggestions; renamed variables for clarity, using a locale helper for the symbol, inlined the padding string expansion.
Still using separate variables for currency_symbol instead of inlineing the call to the multistring locale helper, because there's no operator for e.g. operator+(MultiString, const char*) that would reduce the multistring implicitly to the desired string type. Using ifdefs for the definition of the international name with/without trailing space.
Thanks for this improvement, please update the summary.
In general LGTM, but I would like to consider the duplication before approving.
libcxx/test/std/localization/locale.categories/category.monetary/locale.money.put/locale.money.put.members/put_long_double_zh_CN.pass.cpp | ||
---|---|---|
153 | I'm not too fond of std::string(7 - currency_symbol.size(), ' ') duplicated a lot of times. |
libcxx/test/std/localization/locale.categories/category.monetary/locale.money.put/locale.money.put.members/put_long_double_zh_CN.pass.cpp | ||
---|---|---|
153 | I'm totally ok with e.g. std::string currency_symbol_padding = std::string(7 - currency_symbol.size(), ' '); and using that instead - that would indeed make it less cluttered. (That's what I would prefer here actually, but I'm ok with whatever you guys prefer.) |
LGTM
libcxx/test/support/locale_helpers.h | ||
---|---|---|
91–93 ↗ | (On Diff #412839) | Not a blocker; maybe I just like Unicode character names too much. ;) |
libcxx/test/support/locale_helpers.h | ||
---|---|---|
91–93 ↗ | (On Diff #412839) | Sure, will amend that. (I actually considered adding such comments to begin with, but I didn't find a good reference for the character names with a couple minutes of browsing.) |
Naming nit: By the time I saw it again below, I'd forgotten that curr_text stood for currency_name and not current_string. I suggest spelling out currency_{symbol,name}, or even better, refactoring into a helper function as you suggest in your PR summary.
As in the previous PR, using a MultiStringType helper will eliminate the need for curr_x and w_curr_x to coexist.