Skip to content
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

Merged
merged 4 commits into from
Oct 11, 2019

Conversation

denizevrenci
Copy link
Contributor

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.

@denizevrenci
Copy link
Contributor Author

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.

@vitaut
Copy link
Contributor

vitaut commented Oct 7, 2019

Thanks for the PR. LGTM but there are some CI failures.

@denizevrenci-quoine
Copy link

Yes. They are caused by -Wweak-vtables firing in testing code. I will fix them or find a workaround.

@denizevrenci
Copy link
Contributor Author

denizevrenci commented Oct 8, 2019

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.

@vitaut
Copy link
Contributor

vitaut commented Oct 8, 2019

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

Great find! I think it's a nicer solution than adding defaulted members. Do you know why it results i smaller object files?

@vitaut
Copy link
Contributor

vitaut commented Oct 8, 2019

Thinking more of it, not emitting format_error's vtable may be problematic.

@denizevrenci
Copy link
Contributor Author

denizevrenci commented Oct 10, 2019

Great find! I think it's a nicer solution than adding defaulted members. Do you know why it results in smaller object files?

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 core-test.cc.o 884 bytes bigger and format-test.cc.o 1336 bytes bigger than that of master using clang 9 in MacOS.

I would say it is negligiable considering the total size of 4.8M core-test.cc.o and 15M for format-test.cc.o.

@vitaut
Copy link
Contributor

vitaut commented Oct 10, 2019

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.

@vitaut vitaut merged commit 96f9142 into fmtlib:master Oct 11, 2019
@vitaut
Copy link
Contributor

vitaut commented Oct 11, 2019

Thanks!

@denizevrenci denizevrenci deleted the add_copy_move branch October 11, 2019 18:18
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