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 support for built-in int128 when available #1287

Merged
merged 3 commits into from
Sep 4, 2019

Conversation

denizevrenci
Copy link
Contributor

@denizevrenci denizevrenci commented Aug 29, 2019

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

This is an initial implementation. I want to hear opinions before trying to extend it to other platforms and adding tests for it.

@denizevrenci denizevrenci force-pushed the 6.0.0-mod branch 8 times, most recently from d3174be to 3c76fc7 Compare August 30, 2019 07:38
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! In general looks good, but I have a bunch of inline comments about the details.

include/fmt/core.h Outdated Show resolved Hide resolved
include/fmt/core.h Outdated Show resolved Hide resolved
include/fmt/core.h Outdated Show resolved Hide resolved
include/fmt/core.h Outdated Show resolved Hide resolved
include/fmt/format-inl.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
@denizevrenci denizevrenci force-pushed the 6.0.0-mod branch 2 times, most recently from e0155b1 to 502a163 Compare September 1, 2019 11:35
@denizevrenci
Copy link
Contributor Author

denizevrenci commented Sep 2, 2019

Behavior of std::is_integral is different between libstdc++ with and without GNU extensions, and libc++. I do not want to add specializations to std namespace so I think it is appropriate to have a separate one in the fmt::internal namespace.

I suggest changing the existing functions' name to is_integral_type and is_arithmetic_type to reserve the names of the standard type traits classes.

I noticed that std::is_integral is used in printf.h as well. I suppose at least some of those usages have to be replaced with fmt::internal::is_integral.

Let me know your thoughts.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Not sure how you'd do that. The other block is inside the union.
Do you mean like this?

Nevermind, I forgot that you need to terminate the union which makes merging the ifdefs awkward. Let's keep as is.

Behavior of std::is_integral is different between libstdc++ with and without GNU extensions, and libc++. I do not want to add specializations to std namespace so I think it is appropriate to have a separate one in the fmt::internal namespace.

Makes sense.

I suggest changing the existing functions' name to is_integral_type and is_arithmetic_type to reserve the names of the standard type traits classes.

Sure. So internal::is_integral will become internal::is_integral_type and internal::is_integral_s will become internal::is_integral?

I noticed that std::is_integral is used in printf.h as well. I suppose at least some of those usages have to be replaced with fmt::internal::is_integral.

I don't think printf supports int128_t (at least not portably) so we don't need to change fmt's implementation either because it's mostly for migrating legacy code.

include/fmt/core.h Outdated Show resolved Hide resolved
@denizevrenci
Copy link
Contributor Author

denizevrenci commented Sep 2, 2019

Okay, the warning is on __int128 which is what I used in this PR, not __int128_t.
https://godbolt.org/z/WYAi6I

Looks like __int128_t is aliased to __int128 and __uint128_t is aliased to unsigned __int128. Using those aliases, we can avoid warning suppression.

@vitaut
Copy link
Contributor

vitaut commented Sep 2, 2019

One more thing: please add a test that checks if format works with int128_t somewhere around

TEST(FormatterTest, FormatDec) {
.

@denizevrenci denizevrenci changed the title WIP: Add support for built-in int128 when available Add support for built-in int128 when available Sep 2, 2019
@denizevrenci denizevrenci force-pushed the 6.0.0-mod branch 3 times, most recently from 33bf8b1 to 0a3b7bc Compare September 2, 2019 18:09
To reserve space for the type traits sharing the same name as the ones
from standard library.
include/fmt/format.h Outdated Show resolved Hide resolved
test/format-impl-test.cc Outdated Show resolved Hide resolved
test/format-impl-test.cc Outdated Show resolved Hide resolved
test/format-test.cc Outdated Show resolved Hide resolved
test/format-test.cc Outdated Show resolved Hide resolved
@vitaut
Copy link
Contributor

vitaut commented Sep 4, 2019

Merged, thanks!

@denizevrenci denizevrenci deleted the 6.0.0-mod branch September 4, 2019 14:58
@denizevrenci
Copy link
Contributor Author

Thank you for reviewing and accepting the PR.

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.

2 participants