Skip to content

Add Configuration via CMake and replace all detection macros with BEMAN_ macros - #400

Open
mborland wants to merge 3 commits into
mainfrom
config
Open

mborland wants to merge 3 commits into
mainfrom
config

Conversation

@mborland

@mborland mborland commented Oct 5, 2026

Copy link
Copy Markdown
Member

As requested by @ednolan

@mborland
mborland requested a review from ednolan October 5, 2026 20:25
@mborland
mborland requested a review from eisenwave as a code owner October 5, 2026 20:25
@mborland

mborland commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Once this one is merged, I'll tag a release so that we can add vcpkg and Conan support

@mborland

mborland commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Failures are unrelated to the changes

@eisenwave eisenwave left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This design is clever, but it's not yet complete.

We still have compiler identification that's not covered here. Isn't it possible to install something built with GCC and to then get a different outcome when building that installation with Clang?

Another problem is that lots of our feature detection bypasses the standard feature-test macros entirely by using __has_builtin instead. So things like #if BEMAN_BIG_INT_HAS_BUILTIN(__builtin_add_overflow) presumably violate the Beman standard by doing flag forking as well; these are just compiler version detections under the hood. I don't actually mind merging this now and incrementally fixing the rest of the flag forking though.

@mborland

mborland commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

This design is clever, but it's not yet complete.

We still have compiler identification that's not covered here. Isn't it possible to install something built with GCC and to then get a different outcome when building that installation with Clang?

Theoretically as no version of Clang supports say <stdfloat> while libstdc++ has since 13.

Another problem is that lots of our feature detection bypasses the standard feature-test macros entirely by using __has_builtin instead. So things like #if BEMAN_BIG_INT_HAS_BUILTIN(__builtin_add_overflow) presumably violate the Beman standard by doing flag forking as well; these are just compiler version detections under the hood. I don't actually mind merging this now and incrementally fixing the rest of the flag forking though.

I can pull all of those into config before merge.

@eisenwave

Copy link
Copy Markdown
Collaborator

It's probably best to rip off the bandaid once and fix all the flag forking, including the __has_builtin stuff in this PR.

I assume that's pretty straight forward. The cleanest solution may be to convert code like

#if BEMAN_BIG_INT_HAS_BUILTIN(__builtin_add_overflow)
  __builtin_add_overflow(x, y)
#endif

to

#ifdef BEMAN_BIG_INT_BUILTIN_ADD_OVERFLOW
  BEMAN_BIG_INT_BUILTIN_ADD_OVERFLOW(x, y)
#endif

where BEMAN_BIG_INT_BUILTIN_ADD_OVERFLOW sits in the #else part in config.hpp as well and ends up defined as

#if BEMAN_BIG_INT_HAS_BUILTIN(__builtin_add_overflow)
  #define BEMAN_BIG_INT_BUILTIN_ADD_OVERFLOW(...) __builtin_add_overflow(__VA_ARGS__)
#endif

This branch has not been deployed

No deployments
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