Skip to content
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

Remove macros and a compiler workaround #1307

Merged
merged 19 commits into from
Oct 3, 2020

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Sep 19, 2020

Changelog

  • xlocale, xlocinfo.h
    • Remove _XB and _XS. They were zero, so they served no useful purpose.
    • Move the remaining "ctype code bits" macros from xlocinfo.h to their only usage in xlocale, then immediately #undef them. These macros are extremely short, making them unusually polluting.
  • xstddef
  • ymath.h, src/xmath.hpp
    • Move the "_Feraise argument" macros from ymath.h (where they're visible to users via complex) to src/xmath.hpp (as they're used in separately compiled files only).
  • src/xferaise.cpp
    • Include xmath.hpp to get those macros. (This was the only file not already including xmath.hpp, and it drags in the previously included headers.)
  • functional
    • Remove the workaround for VSO-1132105 which was resolved as fixed.
    • MS-internal testing passed for x86/x64 stl vcr compiler, so it appears that we don't need to retain this workaround for CUDA.
  • deque
    • Call emplace_front (which orphans) and _Emplace_back_internal (which doesn't orphan) in a centralized manner. Note that push_back already behaved like this.
    • Now that they're used exactly once, expand the _PUSH_FRONT_BEGIN etc. macros (deleting unnecessary semicolons). No logic changes.
    • Add missing braces.
    • Replace the macro _DEQUEMAPSIZ with static constexpr int _Minimum_map_size, private within deque.
    • Change the extremely verbose macro _DEQUESIZ to be a static constexpr int in _Deque_val, and rename it to _Block_size now that it's not a macro.
  • complex
    • Comment why the _C_COMPLEX_T guard is necessary.
  • filesystem
    • Change example paths in comments to use cat, dog, elk elements, consistent with tests/std/tests/P0218R1_filesystem/test.cpp. (This isn't strictly required by our policies, but it avoids potential headaches. I also believe that the animals are more readable - they're in alphabetical order and differ in their first letter, unlike the extremely similar bar / baz.)
    • _File_time_clock uses the same 100 nanosecond period as system_clock; simply use that type. (It's not going to change unexpectedly, due to ABI.)
  • chrono, xtimec.h
    • Directly express system_clock::period, removing the last use of _XTIME_NSECS_PER_TICK. Comment that this is 100 nanoseconds.
    • ratio denominators default to 1; taking advantage of this slightly improves clarity.
    • Logic change that should preserve semantics: previously, we were dividing (with truncation) and multiplying by _XTIME_TICKS_PER_TIME_T which was 10000000LL. That value, 10,000,000, is how many 100ns ticks occur in 1s (which is the period for time_t). We can and should use the chrono / ratio type system for this.
      • to_time_t takes a 100ns time_point _Time. _Time.time_since_epoch() gives us a 100ns duration. Previously, we extracted a raw integer with .count(), truncate-divided by _XTIME_TICKS_PER_TIME_T to get a raw integer of whole seconds, and then redundantly cast to __time64_t. (That's the same as our rep, long long.) Now, we can use duration_cast<seconds> to perform the truncating conversion. Then we can extract and return .count(). (seconds is also represented by long long.)
      • from_time_t takes __time64_t _Tm, a raw integer of whole seconds. We previously multiplied by _XTIME_TICKS_PER_TIME_T to get a raw integer of 100ns ticks, then invoked the explicit constructors for our duration and time_point. Now, it's simpler to explicitly construct seconds{_Tm} (again, same long long representation), which encodes in the type system what we know about _Tm's meaning. Then, we can implicitly convert to our duration while explicitly constructing our time_point. (The duration conversion is implicit and safe because converting from whole seconds to fine-grained 100ns ticks is information-preserving. Yay chrono design!)
      • Note that this eliminates redundancy - all of the math is centralized in ratio_multiply<ratio<100>, nano> and we don't need to write down the inverse value _XTIME_TICKS_PER_TIME_T.

Test Coverage

This tests to_time_t's truncating behavior, which is the "hard" part:

void test(const system_clock::time_point& tp, const long long t1, const long long t2) {
assert(tp.time_since_epoch().count() == t1);
assert(system_clock::to_time_t(tp) == t2);
}
int main() {
test(system_clock::time_point(system_clock::duration(13595108447931998LL)), 13595108447931998LL, 1359510844LL);
test(system_clock::time_point(system_clock::duration(13595108501597374LL)), 13595108501597374LL, 1359510850LL);
test(system_clock::time_point::min(),
/* -9223372036854775808LL */ INT64_MIN, -922337203685LL);
test(system_clock::time_point::max(),
/* 9223372036854775807LL */ INT64_MAX, 922337203685LL);
}

This tests from_time_t's behavior:

time_t t = time(nullptr);
STD chrono::system_clock::time_point tp = STD chrono::system_clock::from_time_t(t);
CHECK_INT(t, STD chrono::system_clock::to_time_t(tp));

Therefore, I believe that existing test coverage is sufficient. Just in case, I tested this manually:

Manual Testing

C:\Temp>type meow.cpp
#include <chrono>
#include <iostream>
using namespace std;
using namespace std::chrono;

int main() {
    cout << "_MSVC_STL_UPDATE: " << _MSVC_STL_UPDATE << endl;
    constexpr system_clock::time_point now{system_clock::duration{16004804410890516ll}};
    const auto value = system_clock::to_time_t(now);
    cout << "time_t: " << value << endl;
    const auto round_trip = system_clock::from_time_t(value);
    cout << nanoseconds{now - round_trip}.count() << " ns difference" << endl;
}

VS 2019 16.8 Preview 3:

C:\Temp>cl /EHsc /nologo /W4 /MTd meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202008
time_t: 1600480441
89051600 ns difference

This PR:

C:\Temp>cl /EHsc /nologo /W4 /MTd meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202009
time_t: 1600480441
89051600 ns difference

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Sep 19, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner September 19, 2020 01:41
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/deque Outdated Show resolved Hide resolved
stl/inc/deque Show resolved Hide resolved
@StephanTLavavej
Copy link
Member Author

Moving _INFCODE and _NANCODE will break #1316; I'll restore them.

Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Couldn't find anything to nitpick! Looks good! :)

Copy link
Member

@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.

Pushed a minor merge conflict resolution: #1270 changed #include <errno.h> to #include <cerrno> in xferaise.cpp, which we want to remove here regardless of the header name.

@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants