-
Notifications
You must be signed in to change notification settings - Fork 226
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
Define C++20 concepts #848
base: develop
Are you sure you want to change the base?
Conversation
Some questions:
I'm sure there are more detailed questions that will arise in time, we need to figure out what the general pro's and cons are first. |
Concepts decrease compile times when replacing SFINAE expressions. I expect they will increase if we are replacing
I'll play around with it today. If that doesn't work I was thinking about providing specializations of Boost.Type_Traits in each of Boost.Multiprecision types for both the MP type and it's underlying (if applicable). That would hit a lot of the major types additional types, and then offer some mechanism to disable
I can add one of the special functions to this PR that runs on arbitrary types instead of just builtins as an example. Starting with the ccmath functions offered a simpler starting point.
All of the major compilers support concepts
|
[ci skip]
[ci skip]
[ci skip]
[ci skip]
Here's a small comparison of the impacts running using current develop: Obviously I should run this many more times to be scientific, but there is not obvious divergence between the two. |
@jzmaddock and @NAThompson I think this is good for review and comments. ccmath/abs, beta, and univariate statistics have been converted to use concepts. As usual Apple's Clang needed some help, but everything is green on CI now. |
Thanks Matt, just pulling down the code to experiment with now... |
OK, just messing about with this, I'm seeing a small impact on compile times, which mostly seem to be coming from parsing the new header. I converted log1p.hpp as one of the smallest (most sensitive?) headers to try this out on, and only applied the concepts to the user-callable functions, ie not the code in the detail namespace as IMO that gains nothing from concept checks? Then with a trivial test program:
Build times with -O3 -std=c++20 and cygwin gcc were 4.4s with -DBOOST_MATH_DISABLE_CONCEPTS rising to 5s without the define. Including the new concepts.hpp header but without using them, saw similar compile times to where they were used, so at present this looks like a fixed cost of parsing the header. |
MSVC build times for the same example are a bit variable, but it looks like around 3s with -DBOOST_MATH_DISABLE_CONCEPTS rising to 4s without. |
Confirmed that error messages are much improved though: msvc is quite terse:
While gcc gets a bit more verbose:
|
@jzmaddock I would concur that we probably only need to implement concepts in the user facing functions and not in the detail namespace unless it will save us compile time by replacing SFINAE with concepts. I think it is worth pointing out for posterity that passing a
Do you think this is worth continuing to pursue? I think that a slight increase in compile time is worth it to make our error messages significantly more palatable to the user. |
@jzmaddock How do you want to proceed on this one? I grabbed the changes to log1p off your branch, and CI is green with all of the changes so far. Since the winter development cycle is much longer than the others I think it would be worth merging something close to see what the downstream side effects are on a limited number of functions. |
@jzmaddock, @ckormanyos, and @NAThompson I am looking for some feedback on this PR. This defines new C++20esque concepts for our use, and
abs.hpp
shows both ways to call them to preserve existing SFINAE expressions. I think this would be useful to spread around the codebase to reduce nasty template errors once matured. Thanks for your help.