Skip to content

<format>: std::format should work for volatile arguments #2443

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.

Sign up for GitHub

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

Closed
fsb4000 wants to merge 8 commits into from
Closed

<format>: std::format should work for volatile arguments #2443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ad0da14
format should work for volatile arguments
fsb4000 Dec 21, 2021
4fb5837
Merge branch 'main' into fix2427
fsb4000 Jan 20, 2022
98a6174
cannot use memcpy with volatile types
fsb4000 Jan 20, 2022
12bf147
clang-format
fsb4000 Jan 20, 2022
5ae810a
more precise comment
fsb4000 Jan 20, 2022
096603d
add static assert
fsb4000 Jan 20, 2022
16897f4
Merge branch 'main' into fix2427 and fix conflicts
fsb4000 Jan 26, 2022
186a121
_Phony_basic_format_arg_constructor can work with volatile types
fsb4000 Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion stl/inc/format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1568,7 +1568,16 @@ private:
_Arg_type = _Basic_format_arg_type::_Custom_type;
}

_Store_impl<_Erased_type>(_Arg_index, _Arg_type, static_cast<_Erased_type>(_Val));
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));
}
Comment on lines +1571 to +1580
Copy link
Member

@StephanTLavavej StephanTLavavej Feb 18, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@CaseyCarter CaseyCarter Feb 18, 2022

Choose a reason for hiding this comment

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

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.

}

public:
Expand Down
9 changes: 9 additions & 0 deletions tests/std/tests/P0645R10_text_formatting_args/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,20 @@ void test_visit_monostate() {
assert(visit_format_arg(visitor<Context>, basic_format_arg<Context>()) == Arg_type::none);
}

void test_gh_2427() {
// <format>: volatile integral<T> compilation error
volatile int vol = 42;
auto ret = format("{}", vol);
assert(ret == "42");
}

int main() {
test_basic_format_arg<format_context>();
test_basic_format_arg<wformat_context>();
test_format_arg_store<format_context>();
test_format_arg_store<wformat_context>();
test_visit_monostate<format_context>();
test_visit_monostate<wformat_context>();

test_gh_2427();
}