Fixes case sensitivity when parsing date and time #1168
Conversation
There was a problem hiding this comment.
- Add your test case to tests, see this: Adopt LWG-3390 #567 (comment)
- See why the existing tests are failing and fix
|
I think it will be good if you add to testcases that one(with single 'D'): #1126 (comment) I didn't dig why the existing tests are failing. But I found one regression after your patch: before after Here a list of russian full daynames: short day names: and full month names: and short month names: some examples for testcases with mixed case: Sorry, I don't know how to create unit tests for STL But I think STL need some testcase with locale. |
Use ctype and facet for comparing case-insensitive
Arzaghi
changed the title
|
Any clue why did the tests fail? I tested with these cases and everything seemed fine. #include <iostream>
#include <sstream>
#include <iomanip>
void test_english1() {
std::tm t = {};
std::istringstream ss("2018-dEceMBeR-18");
ss >> std::get_time(&t, "%Y-%b-%d");
if (ss.fail()) {
std::cout << "Parse failed\n";
}
else {
std::cout << std::put_time(&t, "%c") << '\n';
}
}
void test_english2() {
std::tm t = {};
std::istringstream ss("2020-fEb-31 06:59:34pM");
ss >> std::get_time(&t, "%Y-%b-%d %H:%M:%S%p");
if (ss.fail()) {
std::cout << "Parse failed\n";
}
else {
std::cout << std::put_time(&t, "%c") << '\n';
}
}
void test_english3() {
std::tm t = {};
std::istringstream ss("2020-TestaUg-06 11:12:34aM");
ss >> std::get_time(&t, "%Y-Test%b-%d %H:%M:%S%p");
if (ss.fail()) {
std::cout << "Parse failed\n";
}
else {
std::cout << std::put_time(&t, "%c") << '\n';
}
}
void test_russian()
{
std::tm t = {};
std::wstringstream ss;
ss.imbue(std::locale("ru_RU.UTF-8"));
ss << L"2020-Ноя-5";
ss >> std::get_time(&t, L"%Y-%b-%d");
if (ss.fail()) {
std::cout << "Parse failed\n";
}
else {
std::cout << std::put_time(&t, "%c") << '\n';
}
}
void test_german() {
std::tm t = {};
std::wstringstream ss;
ss.imbue(std::locale("de_DE.utf-8"));
ss << L"2015-fEBrUAr-27 08:09:10pM";
ss >> std::get_time(&t, L"%Y-%b-%d");
if (ss.fail()) {
std::cout << "Parse failed\n";
}
else {
std::cout << std::put_time(&t, "%c") << '\n';
}
}
void test_chinese() {
std::tm t = {};
std::wstringstream ss;
ss.imbue(std::locale("chinese"));
ss << L"2019-五月-13 08:09:10pM";
ss >> std::get_time(&t, L"%Y-%b-%d");
if (ss.fail()) {
std::cout << "Parse failed\n";
}
else {
std::cout << std::put_time(&t, "%c") << '\n';
}
}
int main() {
test_english1();
test_english2();
test_english3();
test_russian();
test_german();
test_chinese();
std::cin.get();
return 0;
}
|
|
Click details on check failure, see this in test log: Follow these steps, try to see the same failure locally: Or see further in test log: |
|
|
|
Could you please add tests to |
|
After I fixed the Issues mentioned in Issue #1126 I've just added some tests to cover the scenarios mentioned on the thread. |
|
I would encode those strings as hex literals with comments about content in English, like But probably maintainers should answer |
Tricky solution :) but then those strings won't be readable anymore |
|
Chinese aren't readable to me already. I guess a comment like "\x5432\x2332" /* short monday in Russian */ would suffice. |
|
Or skip Russian and Chinesse, with German it is just a few Umlaut characters |
|
Examples from @BillyONeal |
|
@Arzaghi FYI, I slightly edited your PR description. GitHub supports syntax to link PRs to issues, such that the linked issues show up in the sidebar and will be automatically closed when the PR is merged. However, the syntax is very specific - "Fix for Issue #1126" is treated as an ordinary mention of an issue, while "Fixes #1126." activates the "this PR will close this issue" linking. |
Thank you very much. I didn't know the point. |
There was a problem hiding this comment.
Thanks! I pushed a merge with master, a comment cleanup (we generally prefer comments to be either sentence fragments or complete sentences with proper punctuation, not a mix), and the code movement that @mnatsuhara requested (no other changes) - with _Getloctxt at the end of the file, the diff is much simpler, and we don't need to worry about the placement of class template specializations. I verified locally that this passes tests.
|
Thanks for this significant runtime correctness fix, and the extensive testing! 😸 📆 |
Fixes #1126.