ARROW-15520: [C++] Qualify arrow_vendored::date::format() for C++20 compatibility
#12317
Conversation
|
|
pitrou
changed the title
arrow_vendored::date::format() for C++20 compatibilityarrow_vendored::date::format() for C++20 compatibility
|
Aha, clang-format has rejected overly long lines. (The microsoft/STL repo is also a huge fan of clang-format!) I'll validate and push a commit to fix this. |
|
Although I could have accepted clang-format's wrapping, it seemed ugly (since it consumes extra lines and makes the code not line up nicely). After seeing that namespace aliases are used elsewhere in the codebase, I introduced one here. For consistency, I changed an extra line to use the new alias, considerably shortening it. |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks @StephanTLavavej . I'll wait for CI and merge if green.
|
Benchmark runs are scheduled for baseline = 56386a4 and contender = fb5a4f6. fb5a4f6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
… compatibility As explained in ARROW-15520, these unqualified calls to `format()` are ambiguous in the C++20 Standard. The `using`-declaration `using arrow_vendored::date::format;` makes the compiler consider the desired overload, but it doesn't automatically win. Argument-Dependent Lookup also considers `std::format()` because the arguments are `std::chrono::duration` types (and `<chrono>` includes `<format>` in MSVC's implementation). A very recent change to `std::format()`'s signature in a C++20 Defect Report makes it an equally good match as the desired `arrow_vendored::date::format()` overload, so the compiler emits an ambiguity error. The fix is simple, although slightly verbose - the code simply needs to explicitly qualify each call, in order to defend against Argument-Dependent Lookup. The fix is also perfectly backwards-compatible (i.e. it works in previous Standard versions, and with all other platforms). (Also as mentioned in ARROW-15520, although this requires building Apache Arrow with non-default settings to use the latest C++ Standard version, this change is good for future-proofing and will make it easier for the MSVC team to continue validation that prevents toolset regressions that could affect Apache Arrow and other projects.) Closes apache#12317 from StephanTLavavej/cxx20-format Lead-authored-by: Stephan T. Lavavej <stl@microsoft.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Co-authored-by: Stephan T. Lavavej <stl@nuwen.net> Signed-off-by: Antoine Pitrou <antoine@python.org>
3 participants
As explained in ARROW-15520, these unqualified calls to
format()are ambiguous in the C++20 Standard.The
using-declarationusing arrow_vendored::date::format;makes the compiler consider the desired overload, but it doesn't automatically win. Argument-Dependent Lookup also considersstd::format()because the arguments arestd::chrono::durationtypes (and<chrono>includes<format>in MSVC's implementation). A very recent change tostd::format()'s signature in a C++20 Defect Report makes it an equally good match as the desiredarrow_vendored::date::format()overload, so the compiler emits an ambiguity error.The fix is simple, although slightly verbose - the code simply needs to explicitly qualify each call, in order to defend against Argument-Dependent Lookup. The fix is also perfectly backwards-compatible (i.e. it works in previous Standard versions, and with all other platforms).
(Also as mentioned in ARROW-15520, although this requires building Apache Arrow with non-default settings to use the latest C++ Standard version, this change is good for future-proofing and will make it easier for the MSVC team to continue validation that prevents toolset regressions that could affect Apache Arrow and other projects.)