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

Speculatively implement LWG-3636: formatter<T>::format should be const-qualified #2573

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Feb 15, 2022

@fsb4000 fsb4000 requested a review from a team as a code owner February 15, 2022 14:50
Copy link
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

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

When making changes per non-WP-status lwg issue, you must comment every occurence, mention "per LWG-XXX"

@CaseyCarter CaseyCarter changed the title Implement not approved proposed resolution of LWG-3636: formatter<T>::format should be const-qualified Speculatively implement LWG-3636: formatter<T>::format should be const-qualified Feb 15, 2022
stl/inc/format Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member

CaseyCarter commented Feb 15, 2022

When making changes per non-WP-status lwg issue, you must comment every occurence, mention "per LWG-XXX"

Technically, we require commenting behavior that contradicts the Working Draft. All of these const additions technically don't contradict the Working Draft; it requires some expressions to be valid, they can be made so regardless of whether format is const-qualified.

If we were to start calling format on const program-defined formatters, that would be a different story.

@CaseyCarter CaseyCarter added the LWG Library Working Group issue label Feb 15, 2022
@StephanTLavavej StephanTLavavej added the format C++20/23 format label Feb 15, 2022
Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

Looks good, but the mutable is not good very bad.

Unfortunately we need to just copy _Specs into format before getting the dynamic parameters.

It's not a huge deal as this is only for user-defined types, and if it's a performance problem we can keep the non-const version (assuming wg21 doesn't prohibit that)

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.

LGTM. Added to my "Stuff to port" list.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. (I'll handle verifying that this works with the recent LLVM update.)

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Feb 17, 2022
@StephanTLavavej StephanTLavavej merged commit 7b2c54b into microsoft:main Feb 18, 2022
@StephanTLavavej
Copy link
Member

Thanks for time-traveling from the future when this LWG issue has been accepted! ⌚ 😹 ✅

@fsb4000 fsb4000 deleted the lwg3636 branch February 18, 2022 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format C++20/23 format LWG Library Working Group issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants