Skip to content

Conversation

@eldakesh-ms
Copy link
Contributor

Now we move around the iterator we get instead of copying it. I had to
move a few functions from their home in algorithm and xmemory into
xutility. I hope I got the comments/guards correct, but please double
check those.

The testing is done by making sure we don't use-after-move a custom
iterator and that all the printing functions are instantiated.

@eldakesh-ms eldakesh-ms requested a review from a team as a code owner March 31, 2021 19:49
@CaseyCarter CaseyCarter self-assigned this Mar 31, 2021
@CaseyCarter CaseyCarter added cxx20 C++20 feature format C++20/23 format labels Mar 31, 2021
@CaseyCarter
Copy link
Contributor

I had to move a few functions from their home in algorithm and xmemory into xutility.

Had to "move" them, eh? Good one!

@eldakesh-ms
Copy link
Contributor Author

Squashed and rebased. Double checked that I didn't undo any _NODISCARDs but it's possible I did.


iterator out() {
return _OutputIt;
return _STD move(_OutputIt);
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes me spooked! in particular because basic_format_context and basic_format_parse_context are to be used in user supplied custom formatters

template <class _Ty>
_OutputIt operator()(_Ty _Val) {
return _Write<_CharT>(_Out, _Val);
return _Write<_CharT>(_STD move(_Out), _Val);
Copy link
Contributor

Choose a reason for hiding this comment

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

This means _Default_arg_formatter op()s can also be called only once, like the basic_format_context .out() member function.

basic_format_parse_context<_CharT> _Parse_ctx({});
basic_format_context<_OutputIt, _CharT> _Format_ctx(_Out, _Args, _Loc);
basic_format_context<_OutputIt, _CharT> _Format_ctx(_STD move(_Out), _Args, _Loc);
_Handle.format(_Parse_ctx, _Format_ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

This copy may be problematic.

@eldakesh-ms
Copy link
Contributor Author

There is an outstanding issue with custom formatting. It's not a result of this PR, but running the tests without /SDL will cause issues.

Now we move around the iterator we get instead of copying it. I had to move a few functions from their home in algorithm and xmemory into xutility. I hope I got the comments/guards correct, but please double check those.  The testing is done by making sure we don't use-after-move a custom iterator and that all the printing functions are instantiated.
@eldakesh-ms
Copy link
Contributor Author

Rebased, double check the function moves as those were re-done (xutilty, xmemory, algorithm).

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 am not 100% up to date with fmt but the changes look good for me

@barcharcraz barcharcraz merged commit 334308a into microsoft:feature/format Apr 7, 2021
@cpplearner cpplearner mentioned this pull request Jun 8, 2021
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature format C++20/23 format

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants