Skip to content

Conversation

@vitaut
Copy link
Contributor

@vitaut vitaut commented Apr 25, 2021

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
Copy link
Contributor

SuperWig commented Apr 27, 2021

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

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.

@vitaut vitaut requested a review from barcharcraz April 29, 2021 14:17
@barcharcraz
Copy link
Contributor

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

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

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

@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.

@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

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."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants