Skip to content

Conversation

@CaseyCarter
Copy link
Contributor

... for now, at least. This corrects a defect in our implementation of [format.args]/4's requirement that:

typename Context::template formatter_type<remove_cvref_t<T>>()
  .format(declval<T&>(), declval<Context&>())

be a valid expression. We were incorrectly stripping cv-qualifiers from T.

... for now, at least. This corrects a defect in our implementation of [[format.args]/4](https://eel.is/c++draft/format.arg#4)'s requirement that:
> ```c++
> typename Context::template formatter_type<remove_cvref_t<T>>()
>   .format(declval<T&>(), declval<Context&>())
> ```

be a valid expression. We were incorrectly stripping cv-qualifiers from `T`.
@CaseyCarter CaseyCarter added bug Something isn't working format C++20/23 format labels Feb 18, 2022
@CaseyCarter CaseyCarter requested a review from a team as a code owner February 18, 2022 02:16
@CaseyCarter
Copy link
Contributor Author

I'm open to suggestions for test coverage. I did verify that this program:

#include <format>
#include <iostream>

int main() {
    volatile int v = 42;
    std::cout << std::format("{}\n", v);
}

was accepted before and rejected after the change, but it's hard to do "correctly fails to compile" tests internally.

@fsb4000
Copy link
Contributor

fsb4000 commented Feb 18, 2022

Should your PR also close the issue #2427 ?

Also the issue says that

volatile double v = 42;
std::cout << std::format("{}\n", v);

works.

Should it be rejected too?

Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

I guess we have to wait for libc++ for compile.fail tests

@StephanTLavavej
Copy link
Member

Looks good to me. After reading through the surrounding code, I saw that:

STL/stl/inc/format

Lines 172 to 175 in 7b2c54b

template <class _Ty, class _Context>
concept _Has_formatter = requires(_Ty& _Val, _Context& _Ctx) {
_STD declval<typename _Context::template formatter_type<remove_cvref_t<_Ty>>>().format(_Val, _Ctx);
};

still implements the remove_cvref_t<T> mentioned in https://eel.is/c++draft/format.arg#4 .

@AlexGuteniev
Copy link
Contributor

See also #2246

@CaseyCarter
Copy link
Contributor Author

Should your PR also close the issue #2427 ?

Also the issue says that

volatile double v = 42;
std::cout << std::format("{}\n", v);

works.

Should it be rejected too?

I believe that behavior is consistent with the wording in the working draft; we should accept volatile float/double/long double/const char_type*/nullptr_t/cv-void* which are copied into non-volatile storage before formatting, otherwise only volatile program-defined class types whose formatter accepts volatile objects. I don't necessarily agree that the wording is sensible, but that is what it now says.

@barcharcraz barcharcraz removed their assignment Mar 4, 2022
@StephanTLavavej StephanTLavavej self-assigned this Mar 4, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 4bb41db into microsoft:main Mar 4, 2022
@StephanTLavavej
Copy link
Member

Thanks for handling this highly volatile issue! 🧪 🥼 😹

@CaseyCarter CaseyCarter deleted the fmt branch January 4, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working format C++20/23 format

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants