-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add defaulted copy and move operations to format_error and system_error #1347
Conversation
Looks like there are other instances of weak vtables in the test code. It could be fixed with some boilerplate but I am not sure if it is worth it. |
Thanks for the PR. LGTM but there are some CI failures. |
Yes. They are caused by |
I noticed it is possible to suppress the warnings and reduce the sizes of the object files by adding an undefined private virtual member function as follows: private:
virtual void avoid_weak_vtable(); This can be used in both test code and the two exceptions that I added the defaulted special members to. It avoids the boilerplate associated with adding defaulted members for rule of 5. |
Great find! I think it's a nicer solution than adding defaulted members. Do you know why it results i smaller object files? |
Thinking more of it, not emitting |
e62ec3f
to
eaf505d
Compare
I noticed later that it needs the definition of the virtual function even though it is never used. So, this "solution" with the definitions included created I would say it is negligiable considering the total size of 4.8M |
The test sizes don't matter but the library size does. There were requests to make it smaller and we'll do it at some point. Also adding an unused function is a bit of a hack. I suggest going to the previous version of the PR with defaulted members. |
Compiler generated copy operations are deprecated and move operations are not generated altogether.
eaf505d
to
923e798
Compare
Thanks! |
Another approach to not trigger
-Wdeprecated
and-Wweak-vtables
after the discussion in #1334.I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.