codecov[bot]
commented
7 months ago Codecov Report
Attention: 4 lines in your changes are missing coverage. Please review.
Comparison is base (
941f853) 95.66% compared to head (1bd9b6c) 95.71%.
Closed Flamefire closed 7 months ago
codecov[bot]
commented
7 months ago Attention: 4 lines in your changes are missing coverage. Please review.
Comparison is base (
941f853) 95.66% compared to head (1bd9b6c) 95.71%.
artyom-beilis
commented
7 months ago Small but important note: if the facet isn't derived from std::collate the std::locale::operator() can no longer perform its function. https://en.cppreference.com/w/cpp/locale/locale/operator()
the std::locale can be used as replacement for std::less for string comparison.
Flamefire
commented
7 months ago Yes but we have 2 things here: std::collate implementation with "proper unicode support" and boost::locale::collator which is similar to std::collate but provides support for collation levels, used by e.g. comparator
I split those 2 into 2 classes because not all backends implement boost::locale::collator. Where they do an adapter class is used deriving from std::collate to perform the function you mention.
artyom-beilis
commented
7 months ago Yes I understand that boost::locale::collator is much better. But the fact that the facet is derived from std::collate allows to use something that is provided by standard library (even lesser so):
// standard
std::locale loc = std::locale("ru_RU.UTF-8");
// boost
boost::locale::generator gen;
std::locale loc = gen("ru_RU.UTF-8");
// same for both variants
std::sort(v.begin(), v.end(), loc);Something that was direct upgrade path from existing locales (BTW collation is something that actually works with standard locale on Linux quite ok)
I don't know how many users will actually be affected but this is something to think about - if it is worth breaking the code.
Flamefire
commented
7 months ago But the fact that the facet is derived from
std::collateallows to use something that is provided by standard library (even lesser so):
Being derived from std::collate implementing additional virtual methods leads to actual, subtile bugs -> See #215
Hence the change to make this safer
// standard std::locale loc = std::locale("ru_RU.UTF-8"); // boost boost::locale::generator gen; std::locale loc = gen("ru_RU.UTF-8"); // same for both variants std::sort(v.begin(), v.end(), loc);
This still works as mentioned before. See especially https://github.com/boostorg/locale/pull/216/files#diff-9061a93b241603967564b57e006fdb1956988bbc54d82af2d5d3228e99634914
Something that was direct upgrade path from existing locales (BTW collation is something that actually works with standard locale on Linux quite ok)
What exactly "works with standard locale on Linux quite ok"? Is that affected by this PR anyhow?
I don't know how many users will actually be affected but this is something to think about - if it is worth breaking the code.
I don't think this affects many users. The only ones where code will be broken is if they assume boost::locale::collator derives from std::collator which will now fail to compile.
Other code where there previously was a type confusion will now correctly throw a std::bad_cast as expected.
All other code should continue to work as before as confirmed by the test_collate and test_winapi_collate tests
The class derived from
std::collatewhich is always present instd::locale. So checks for theboost::locale::collatorfacet may return wrongly true.As a fix make this an independent facet with its own ID. Use an adapter such that a std::collate derived class can use its abilities.
Add tests to detect the issue and verify the fix.
Fixes #215