<format>: std::format should work for volatile arguments
#2443
Conversation
|
I'm concerned this is not correctly handling reads from volatile objects - it's casting away volatile and using |
|
I saw Charlie had changed |
|
It's not clear what a pull request to fix this could do without kinda breaking the spirit of |
|
I agree that this is a little sloppy now because I started this PR before your PR was merged: #2323 and then I delete part of my PR. https://stackoverflow.com/a/36729638
So now, the PR tries to avoid read the volatile variable multiple times as @CaseyCarter noticed. We just do a copy, and the compiler knows how to work with volatile variables so it will touch the variable only once and then we pass reference to copy and Also I decided to keep the regression test from issue 2427. At least that's my intention in this PR. |
| if constexpr (is_volatile_v<_Ty>) { | ||
| // _Erased_type is not a volatile type, so static_cast<_Erased_type> would cast away volatile | ||
| // _Store_impl uses memcpy for storing the value. We cannot use memcpy with volatile types. | ||
| // Because of the two reasons we have to make a temporary | ||
| static_assert(!is_volatile_v<_Erased_type>); | ||
| const _Erased_type _Temp = _Val; | ||
| _Store_impl<_Erased_type>(_Arg_index, _Arg_type, _Temp); | ||
| } else { | ||
| _Store_impl<_Erased_type>(_Arg_index, _Arg_type, static_cast<_Erased_type>(_Val)); | ||
| } |
There was a problem hiding this comment.
While video reviewing this, @barcharcraz and @CaseyCarter helped me understand the history of this code and what the Standard says. After looking at this, we believe that volatile int is required to be ill-formed by the Standard, according to https://eel.is/c++draft/format.arg#4 . Specifically,
typename Context::template formatter_type<remove_cvref_t<T>>()
.format(declval<T&>(), declval<Context&>())will pass declval<volatile int&>() to formatter_type<int>, and that will attempt to bind .format()'s parameter const int& to the lvalue argument of type volatile int, and that is not allowed.
@barcharcraz says that after #2323, the example in #2427 will be accepted (with a technically-undefined memcpying from volatile), but the Standard says we should reject this, so the code needs to be changed. I'm not immediately sure what the best change would be, but static_assert(!is_volatile_v<remove_reference_t<_Ty>>) (warning: not compiled, I just typed that out) might be a minimal fix. @barcharcraz and @CaseyCarter can help investigate.
There was a problem hiding this comment.
While it doesn't require library-defined formatter specializations to do so, I think the wording may allow a program-defined formatter specialization to accept volatile arguments. The stripping of volatile in [format.arg]/17.2 suggests to me that the drafters probably just didn't care enough to allow or forbid volatile.
|
right solution: #2579 |
Fixes #2427