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 std::is_constant_evaluated support. #394

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jzmaddock
Copy link
Collaborator

No description provided.

Comment on lines +1095 to +1105
[[`BOOST_IS_CONSTANT_EVALUATED_INT(integer-expression)`][Evaluates to `true` when `integer-expression` is `constexpr`.
The macro may be functional in C++14 onwards if the compiler has a suitable intrinsic, in particular this macro is functional for gcc-6 and onwards
as well as for compilers which have direct support for `std::is_constant_evaluated`.

Note that before using this macro, the program must include `<type_traits>`.

Note that this macro may behave subtly different from `std::is_constant_evaluated()` in that it may evaluate to
`true` when code is evaluated in a non-constexpr context, but is none the less evaluated at "compile time" as a result
of program optimisation.

The macro always evaluates to `false` on unsupported/old compilers.]]
Copy link

Choose a reason for hiding this comment

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

From what I see Implementation in some cases uses std::is_constant_evaluated , in some __builtin_constant_p. I found no guarantees in documentation that __builtin_constant_p must return true in constexpr evaluation. Intuitively during constexpr function evaluation it should always return true, but I am not sure that is guaranteed. Consider this tricky case .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nod. There will certainly be tricky cases where you can abuse it into doing the wrong thing, if it's easier I could just omit that macro.

Choose a reason for hiding this comment

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

if it's easier I could just omit that macro.

Hard for me to judge.
My suggestion is to ask the users what they feel. I would personally never use it since I consider it too tricky, but I am not really an expert library developer and I have no usecases for it.

Copy link
Member

Choose a reason for hiding this comment

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

__builtin_constant_p(n) is not a constant expression, and it does not indicate whether n is a constant expression either. See here. What it shows is whether the compiler is able to deduce the value of n at compile time, including as a result of optimizations (e.g. inlining). This means that its value may change depending on optimization level used. If it returns true, n may not be a constant expression in C++ definition and thus may not be used in C++ constant expressions.

I think BOOST_IS_CONSTANT_EVALUATED_INT needs to be renamed to avoid confusion with std::is_constant_evaluated. __builtin_constant_p is useful, but it is a very different thing.


[table
[[macro][Description]]
[[`BOOST_IS_CONSTANT_EVALUATED()`][Evaluated to `true` when the current context is `constexpr` and false otherwise.
Copy link

Choose a reason for hiding this comment

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

beginner question: why is this a macro?

I think boost::is_constant_evaluated() would be a bit nicer, I presume this is for consistency with other stuff in this repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We would need to include <type_traits> to cover that, and I'd very much like to NOT do that.

Explanation: if it's a compiler about which we know nothing about, ie we don't know what it calls the compiler intrinsic that implements this, then we would need to be able to forward to std::is_constant_evaluated if it's presence is advertised in .

As it stands, it's up to users of these macros to include <type_traits> and everyone else can do without if they wish and only pay for what they use.

We could of course put a real function into Boost.Core.

Choose a reason for hiding this comment

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

Ah, I see, I was naively assuming that that feature macro is predefined, now I see it is in <type_traits>. Shame.

@@ -0,0 +1,60 @@
// (C) Copyright John Maddock 2021.

Choose a reason for hiding this comment

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

Would it be good to add a "sanity test"?

For some compiler you know it should have proper machinery in C++20 mode? I am not sure if boost tests have modern compilers running with -std=c++20, but I was thinking about something like this.

https://www.godbolt.org/z/r9Y5x3x6M

(use of assert just as an example, it can be a test framework assert_eq in real code)

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