Implement a packed format store #1625

miscco · 2021-02-05T12:40:28Z

This works towards adding a packed basic_format_arg_store

It is almost fully functional, however some things are missing:

I did not yet include the handle constriants as I focussed on getting the basic types right
We should use a smaller unsigned type for the _Format_arg_store_packed_index
When writing this the default format_context definitions were not available

…debug optimization

…ression

Co-authored-by: S. B. Tam <cpplearner@outlook.com>

miscco · 2021-02-05T12:59:12Z

Note I did not test the value of the arguments

miscco

Shouldn't this go to format branch?

Yeah, I messed up, should be correct now

stl/inc/format

CaseyCarter · 2021-02-05T17:50:28Z

stl/inc/format

+            if (_Id >= _Num_args) {
+                _You_see_this_error_because_arg_id_is_out_of_range();
+            }


Should this be _STL_VERIFY(_Id < _Num_args, "No, silly - don't do that")?

I am totally fine with anything that gets the job done and helps the user understand the error

afaik the reason libfmt doesn't do this check unconditionally at runtime is that it gets caught later on anyway.

That said, is STL_VERIFY always active and how does the codegen compare against a simple function call?

it looks to me like STL_VERIFY is always enabled (and STL_ASSERT expands to it in debug mode)

So what I do not like, is that the STL_VERIFY error message is really bad from an error reporting stand point.

It essentially first complains about wassert and you do not get the full error message.

In contrast that function would be always directly flagged by the compiler.

Note that nothing happens during runtime right now. If we want to change that STL_VERIFY would make more sense

stl/inc/format

CaseyCarter · 2021-02-05T18:45:16Z

stl/inc/format

+    void _Store(const size_t _Arg_index, basic_string<_CharType, _Traits, _Alloc> _Val) noexcept {
+        _Store_impl<basic_string_view<_CharType>, _Basic_format_arg_type::_String_type>(
+            _Arg_index, _Basic_format_arg_type::_String_type, basic_string_view<_CharType>{_Val.data(), _Val.size()});
+    }


I get that the previous two functions are overloads because they need to deduce the parameters of basic_string_view/basic_string, but it seems the overloads that take float/double/long double/const _CharType*/nullptr_t could be part of the if constexpr stack in the first overload.

they are depicted as overloads in the standard, I'm not actually sure why.

I suppose the floating point overloads can be used even if the formatter associated with float/double/long double doesn't meat the formatter requirements.

Moved arithmetic types to their own function

CaseyCarter · 2021-02-05T18:48:51Z

stl/inc/format

+    // clang-format off
+    template <class _Ty>
+        requires is_void_v<_Ty>
+    void _Store(const size_t _Arg_index, const _Ty* p) noexcept {


Is this function intended to accept something other than void* and const void*? If not, could it simply be a non-template that takes const void*? If it's intended to accept volatile void* and const volatile void*, the static_cast on 641 is ill-formed (static_cast can't cast away cv-qualifiers).

I think It is explicitly made to prevent volatile and discriminate between string literals.

That said I do not know why it was given in the standard like that https://eel.is/c++draft/format#arg-12

stl/inc/format

tests/std/tests/P0645R10_text_formatting_args/test.cpp

barcharcraz · 2021-02-05T22:23:26Z

stl/inc/format

+        : _Index(static_cast<_Index_type>(_Index)), _Type(_Basic_format_arg_type::_None) {}
+
+    _Index_type _Index : (sizeof(_Index_type) * 8 - 4);
+    _Basic_format_arg_type _Type : 4;


note to self: this is better than
{
data[]
offset[]
tag[]
}
because tag is half a byte and this way we don't pull in useless tags when we load the one we want

miscco

Note to self we are missing enums

stl/inc/format

miscco · 2021-02-06T19:31:15Z

stl/inc/format

+    byte _Storage[(_STD max)(_Storage_length, 1ull)];
+    _Index_type _Index_array[(_STD max)(_Num_args, 1ull)];


Note that this is essentially equivalent to the standards variant array

we just move the _Index member of the variant into its own array.

That said I think we should move to a type erased storage struct, and pass a pointer to that to basic_format_args.

That way we would only need one pointer rather than two

stl/inc/format

+        } else if constexpr (signed_integral<_Ty> && sizeof(_Ty) <= sizeof(int)) {
+            _Store_impl<int, _Basic_format_arg_type::_Int_type>(_Arg_index, static_cast<int>(_Val));
+        } else if constexpr (unsigned_integral<_Ty> && sizeof(_Ty) <= sizeof(unsigned)) {
+            _Store_impl<unsigned, _Basic_format_arg_type::_UInt_type>(_Arg_index, static_cast<unsigned>(_Val));


stl/inc/format

+    void _Store(const size_t _Arg_index, basic_string<_CharType, _Traits, _Alloc> _Val) noexcept {
+        _Store_impl<basic_string_view<_CharType>, _Basic_format_arg_type::_String_type>(
+            _Arg_index, _Basic_format_arg_type::_String_type, basic_string_view<_CharType>{_Val.data(), _Val.size()});
+    }


stl/inc/format

tests/std/tests/P0645R10_text_formatting_args/test.cpp

stl/inc/format

miscco and others added 9 commits January 29, 2021 20:27

Noralize next_arg_id

5191e21

Add precondition check to basic_format_parse_context::advance_to and …

6a4ff26

…debug optimization

ensure basic_format_parse_context::check_arg_id is not a constant exp…

be1afc0

…ression

Add tests for basic_format_parse_context

ae417d7

Fix constant expression check

cbea0a5

Not not

ba63570

windsdk is bad

816fe40

Address review comments

5485658

Use a non constexpr function call with a better name

e938cfa

Co-authored-by: S. B. Tam <cpplearner@outlook.com>

miscco requested a review from a team as a code owner February 5, 2021 12:40

miscco force-pushed the format_store branch from d3aba32 to d380d33 Compare February 5, 2021 12:53

This comment has been minimized.

Sign in to view

miscco changed the base branch from main to feature/format February 5, 2021 12:59

Create a packed _Format_arg_store

31b1599

miscco force-pushed the format_store branch from d380d33 to 31b1599 Compare February 5, 2021 13:01

miscco commented Feb 5, 2021

View reviewed changes

miscco added 3 commits February 5, 2021 14:41

We want bits not bytes

94f7109

No shadowing

6fc4e47

Dont use friendship

6e46b07

miscco force-pushed the format_store branch from 89b284e to e48806f Compare February 5, 2021 17:35

Remove all int128 code

2700e2a

miscco force-pushed the format_store branch from e48806f to 2700e2a Compare February 5, 2021 18:50

Fix some bugs

dd88cc0

CaseyCarter reviewed Feb 5, 2021

View reviewed changes

miscco added 2 commits February 5, 2021 20:31

Who doesnt hate max/min

218ffc1

Copy&Paste does not like ODR

abb61b1

barcharcraz reviewed Feb 5, 2021

View reviewed changes

CaseyCarter mentioned this pull request Feb 5, 2021

std::format parsing callbacks and machinery #1616

Merged

StephanTLavavej added the cxx20 C++20 feature label Feb 5, 2021

miscco commented Feb 6, 2021

View reviewed changes

miscco added 2 commits February 7, 2021 09:48

Int is in our hearts

d50f51d

Remove duplication

a152b99

miscco force-pushed the format_store branch 3 times, most recently from b360a5f to 971b137 Compare February 7, 2021 21:23

Move _CharType alias into function to unblock the build

87b4851

miscco force-pushed the format_store branch from 971b137 to 87b4851 Compare February 8, 2021 07:19

CaseyCarter reviewed Feb 8, 2021

View reviewed changes

stl/inc/format Outdated Show resolved Hide resolved

stl/inc/format Show resolved Hide resolved

stl/inc/format Outdated Show resolved Hide resolved

stl/inc/format Outdated Show resolved Hide resolved

CaseyCarter reviewed Feb 8, 2021

View reviewed changes

stl/inc/format Outdated Show resolved Hide resolved

barcharcraz reviewed Feb 9, 2021

View reviewed changes

stl/inc/format Outdated Show resolved Hide resolved

barcharcraz reviewed Feb 9, 2021

View reviewed changes

stl/inc/format Outdated Show resolved Hide resolved

CaseyCarter reviewed Feb 9, 2021

View reviewed changes

stl/inc/format Outdated Show resolved Hide resolved

StephanTLavavej reviewed Feb 10, 2021

View reviewed changes

miscco added 3 commits February 10, 2021 21:24

First round of review comments

3caf20e

Use variable template rater than function

c5c8bbc

Add warning

f3c91d0

barcharcraz self-assigned this Feb 10, 2021

miscco added 3 commits February 11, 2021 21:01

Make a proper constructor of _Format_arg_store

8afe67b

Use a single contiguous array to store the _Format_arg_store data

06cb9e9

Specialize empty _Format_arg_store

302ac20

StephanTLavavej reviewed Feb 12, 2021

View reviewed changes

stl/inc/format Show resolved Hide resolved

stl/inc/format Outdated Show resolved Hide resolved

stl/inc/format Outdated Show resolved Hide resolved

stl/inc/format Outdated Show resolved Hide resolved

More review comments

9f85da5

miscco force-pushed the format_store branch from dea113c to 946e320 Compare February 12, 2021 07:09

Fix uninitialized memory

70fc37b

miscco force-pushed the format_store branch from 946e320 to 70fc37b Compare February 12, 2021 07:09

Properly initialize the first Index

76b8a55

barcharcraz approved these changes Feb 12, 2021

View reviewed changes

barcharcraz merged commit a52214a into microsoft:feature/format Feb 12, 2021

miscco deleted the format_store branch February 12, 2021 14:49

		byte _Storage[(_STD max)(_Storage_length, 1ull)];
		_Index_type _Index_array[(_STD max)(_Num_args, 1ull)];

Implement a packed format store #1625

Implement a packed format store #1625

Uh oh!

Conversation

miscco commented Feb 5, 2021

Uh oh!

This comment has been minimized.

miscco commented Feb 5, 2021

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CaseyCarter Feb 5, 2021 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CaseyCarter Feb 5, 2021 •

edited by StephanTLavavej

Loading