Skip to content

Prepare for adding compile-time checks in <format>#1882

Closed
vitaut wants to merge 5 commits intomicrosoft:mainfrom
fmtlib:string
Closed

Prepare for adding compile-time checks in <format>#1882
vitaut wants to merge 5 commits intomicrosoft:mainfrom
fmtlib:string

Conversation

@vitaut
Copy link
Contributor

@vitaut vitaut commented Apr 25, 2021 •
edited
Loading

This PR prepares the formatting API for adding compile-time checks. Specifically, it adds a format string type, _Basic_format_string and updates formatting functions to use it instead of basic_string_view. The constructor of _Basic_format_string is constexpr instead of consteval because of a MSVC issue: https://godbolt.org/z/aMrbe6xzn.

It implements most of the first part of P2216 and is relative to #1874.

@vitaut vitaut requested a review from a team as a code owner April 25, 2021 13:56
@StephanTLavavej StephanTLavavej added enhancement Something can be improved format C++20/23 format labels Apr 25, 2021
SuperWig
SuperWig reviewed Apr 26, 2021
View reviewed changes
miscco
miscco reviewed Apr 26, 2021
View reviewed changes
AdamBucior
AdamBucior reviewed Apr 26, 2021
View reviewed changes
@SuperWig
Copy link
Contributor

SuperWig commented Apr 27, 2021 •
edited
Loading

Am I blind or are the accidental removals of

template <class... _Types>

from a couple functions not in the diff?

@vitaut
Copy link
Contributor Author

vitaut commented Apr 27, 2021

Am I blind or are the accidental removals of ... from a couple functions not in the diff?

I'm not sure what you are referring to, please comment on specific line. All the _Types parametrization is still there AFAICS.

@SuperWig
Copy link
Contributor

SuperWig commented Apr 27, 2021 •
edited
Loading

Oh nvm you changed concrete types, that's why.
But this is why the tests are failing

STL/stl/inc/format

Lines 2955 to 2961 in f0226c2

_NODISCARD inline string vformat(const _Fmt_string<_Types...> _Fmt, const format_args _Args) {
string _Str;
_Str.reserve(_Fmt._Str.size() + _Args._Estimate_required_capacity());
_STD vformat_to(_STD back_inserter(_Str), _Fmt._Str, _Args);
return _Str;
}

_Types... is?

@vitaut
Copy link
Contributor Author

vitaut commented Apr 27, 2021

the tests are failing

Yeah, I accidentally changed wrong overloads. Fixed now.

@StephanTLavavej StephanTLavavej mentioned this pull request Apr 28, 2021
Merged
StephanTLavavej
StephanTLavavej approved these changes Apr 28, 2021
View reviewed changes
barcharcraz
barcharcraz suggested changes Apr 29, 2021
View reviewed changes
@vitaut vitaut requested a review from barcharcraz April 29, 2021 14:17
@barcharcraz
Copy link
Contributor

barcharcraz commented May 2, 2021

This needs a rebase after the debloating changes were merged.

@vitaut
Copy link
Contributor Author

vitaut commented May 3, 2021

Rebased.

@barcharcraz
Copy link
Contributor

barcharcraz commented May 5, 2021

Let's mark this PR WIP until the feature is done. We're happy to review it incrementally, but don't want to merge a partial implementation of the paper (this is because things in our main branch will flow into future Visual Studio releases, and we'd prefer not to release a partial feature even if it doesn't break anything. That way we have more ability to change implementation strategies partway through).

@barcharcraz
Copy link
Contributor

barcharcraz commented May 5, 2021

I know we merged the "other" part of P2216 separately, but that was really it's own feature (and it would have been nice if it had been it's own paper).

@fsb4000
Copy link
Contributor

fsb4000 commented Jun 17, 2021

@barcharcraz I saw in https://github.com/microsoft/STL/projects/6 that need to cite DevCom bug. I found it's easy for me.
https://developercommunity.visualstudio.com/t/consteval-constructor-and-C7595/1453392
We could do it in April :(

@barcharcraz
Copy link
Contributor

barcharcraz commented Jun 22, 2021 •
edited by StephanTLavavej
Loading

@barcharcraz I saw in https://github.com/microsoft/STL/projects/6 that need to cite DevCom bug. I found it's easy for me.
https://developercommunity.visualstudio.com/t/consteval-constructor-and-C7595/1453392
We could do it in April :(

thanks, that looks like it is indeed the correct bug. I'll make sure the internal version of that bug (VSO-1344623) is categorized correctly so folks know stl features depend on it.

@fsb4000 fsb4000 mentioned this pull request Jul 4, 2021
Closed
@StephanTLavavej StephanTLavavej added the blocked Something is preventing work on this label Jul 14, 2021
@vitaut
Copy link
Contributor Author

vitaut commented Aug 2, 2021

Closing as I don't plan to work on this PR.

@vitaut vitaut closed this Aug 2, 2021
@barcharcraz
Copy link
Contributor

barcharcraz commented Aug 3, 2021

Closing as I don't plan to work on this PR.

Thanks, I'll pick it up

@fsb4000
Copy link
Contributor

fsb4000 commented Aug 8, 2021

@barcharcraz https://developercommunity.visualstudio.com/t/consteval-constructor-and-C7595/ is fixed.
"A fix for this issue has been internally implemented and is being prepared for release. We'll update you once it becomes available for download."

@barcharcraz barcharcraz mentioned this pull request Sep 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@barcharcraz barcharcraz Awaiting requested review from barcharcraz

+3 more reviewers

@SuperWig SuperWig SuperWig left review comments

@miscco miscco miscco left review comments

@AdamBucior AdamBucior AdamBucior left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocked Something is preventing work on this enhancement Something can be improved format C++20/23 format

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vitaut @SuperWig @barcharcraz @fsb4000 @miscco @StephanTLavavej @AdamBucior