Skip to content

Conversation

@Arzaghi
Copy link
Contributor

@Arzaghi Arzaghi commented Sep 8, 2020

Fixes #944

@Arzaghi Arzaghi requested a review from a team as a code owner September 8, 2020 21:00
@Arzaghi Arzaghi changed the title <iomanip>: fixes get_time parse format without delimiters <iomanip>: fixes get_time to parse without delimiters Sep 8, 2020
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 9, 2020
@StephanTLavavej StephanTLavavej removed their assignment Sep 24, 2020
@Arzaghi
Copy link
Contributor Author

Arzaghi commented Sep 24, 2020

An irrelevant issue I encountered during fixing this issue. I generally use clang-format extension for Visual Studio and apply the .clang-format file using Tools->Clang Format Document. But this option format these lines as follow:

        const int _Hi_digits = (_Hi <= 9      ? 1
                                : _Hi <= 99   ? 2
                                : _Hi <= 999  ? 3
                                : _Hi <= 9999 ? 4
                                              : static_cast<int>(_STD log10(_STD abs(_Hi))) + 1);

which looks very clear and easy to read.
But the clang-format validation on the sever reject them and expect those lines to be as follow:

STL/stl/inc/xloctime

Lines 542 to 545 in 40f2339

const int _Hi_digits =
(_Hi <= 9 ? 1
: _Hi <= 99 ? 2
: _Hi <= 999 ? 3 : _Hi <= 9999 ? 4 : static_cast<int>(_STD log10(_STD abs(_Hi))) + 1);

I use the latest version of Clang-Format and the VS extension on my system. I'm wondering about the root of this difference and I'm not sure whether something wrong with a configuration on my system or this happening because of the difference between Clang-Format versions on my system and the server.
By the way, I encountered some other similar minor conflicts on my previous PRs.

@StephanTLavavej
Copy link
Member

Different versions of clang-format do change behavior; we are currently using 10.0.0. (Failed clang-format checks will print out the version used; it's the one installed by the VS Preview that we're using.)

In theory, if you're using the same version, and you're using the --style=file option to pick up our .clang-format configuration, the results should be deterministically identical.

@StephanTLavavej StephanTLavavej self-assigned this Oct 7, 2020
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

This looks good to me - I especially like the set of test cases you've crafted. I'll push a change to simplify the _Hi_digits computation and move this forward to Final Review 😸

@StephanTLavavej StephanTLavavej removed their assignment Oct 27, 2020
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Impressively exhaustive tests!

@Arzaghi
Copy link
Contributor Author

Arzaghi commented Oct 28, 2020

@StephanTLavavej @CaseyCarter
I've added one more tiny (yet important) test case to have better coverage for this scenario.
Excuse me I'm adding this test case at the final stage and after your review and approval.

@StephanTLavavej
Copy link
Member

Thanks - both for the additional test case of trailing non-digit characters, and the heads up 😸

@StephanTLavavej StephanTLavavej self-assigned this Oct 29, 2020
@StephanTLavavej StephanTLavavej merged commit 372021d into microsoft:master Oct 29, 2020
@StephanTLavavej
Copy link
Member

Thanks again for fixing this widely reported runtime correctness bug! Your fix will ship in VS 2019 16.9 Preview 2. 🐈

@Arzaghi Arzaghi deleted the Fix_Issue944 branch October 29, 2020 07:38
@randydu
Copy link

randydu commented Feb 2, 2021

The VS 2019 Preview 3 (Visual Studio 2019 Developer Command Prompt v16.9.0-pre.3.0) is installed but the following code cannot parse iso-string properly:

    std::tm tm{0};
    std::istringstream ss(str);
    ss >> std::get_time(&tm, "%Y%m%dT%H%M%S");
    if (ss.fail())
        throw std::invalid_argument("Invalid argument: " + str);

   //str = "19700101T000001"

Expected:

  CHECK(tm.tm_sec == 1); //should be 1 second
  CHECK(tm.tm_min == 0);

Actual:

  CHECK(tm.tm_sec == 0);
  CHECK(tm.tm_min == 1); // 1 min ?

The same test works fine for gcc and clang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<iomanip>: get_time should parse format without delimiters

5 participants