Skip to content

Don't explicitly delete copy ctor of dynamic_format_arg_store - #2664

Merged
vitaut merged 3 commits into
fmtlib:masterfrom
lucpelletier:fix
Dec 23, 2021
Merged

vitaut merged 3 commits into
fmtlib:masterfrom
lucpelletier:fix

Conversation

@lucpelletier

Copy link
Copy Markdown
Contributor

Explicitly deleting the copy ctor causes the move constructor to not be
implicitly generated. This behaviour is different than what was in
v8.0.1 and causes code that relied on the move ctor of
dynamic_format_arg_store to break.

Explicitly deleting the copy ctor causes the move constructor to not be
implicitly generated. This behaviour is different than what was in
v8.0.1 and causes code that relied on the move ctor of
dynamic_format_arg_store to break.
@vitaut

vitaut commented Dec 18, 2021

Copy link
Copy Markdown
Contributor

Thanks for the PR. Could you elaborate why you need a move ctor?

@lucpelletier

Copy link
Copy Markdown
Contributor Author

Yes, of course. I want to use Quill, which is a low-latency asynchronous logging library that uses fmtlib. The following class has a member of type dynamic_format_arg_store, and needs to support move operations:

https://github.com/odygrd/quill/blob/1386bff42eea9411600e58b9ddd682f1ec685c5a/quill/include/quill/detail/backend/BackendWorker.h#L211

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense but please add a unit test for the move ctor in https://github.com/fmtlib/fmt/blob/master/test/args-test.cc.

@lucpelletier

Copy link
Copy Markdown
Contributor Author

Added test for move ctor. Did you also want a test for move assignment?

Comment thread test/args-test.cc Outdated
const char* const test_c_string = "foo";

auto store_uptr =
std::make_unique<fmt::dynamic_format_arg_store<fmt::format_context>>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

std::make_unique - C++14+

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@vitaut
vitaut merged commit 7812813 into fmtlib:master Dec 23, 2021
@vitaut

vitaut commented Dec 23, 2021

Copy link
Copy Markdown
Contributor

Thank you

@lucpelletier
lucpelletier deleted the fix branch December 24, 2021 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants