-
Notifications
You must be signed in to change notification settings - Fork 133
Constexpr additions #485
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
base: master
Are you sure you want to change the base?
Constexpr additions #485
Conversation
@JustCallMeRay this pull request is interesting, Have you time to complete it? |
Hiya, sorry work happens I have less time to contribute to open source stuff. So the constexpr ends at the graph class because it uses std::unordered_map which is not constexpr. There are third party alternatives to the standard which are better for certain things than the standard, for example frozen I think would do it. However if the user does not want to use frozen (say it's slower or doesn't work on embedded systems for example), so I had thought about having cmake configure a simple header which would be something like But didn't get around to it. |
I had a quick look at the frozen benchmarks and they look much quicker than #if __has_include(<frozen/unordered_map.h>)
# include <frozen/unordered_map.h>
# define CXXGraph_ConstexprMap constexpr
namespace CxxGraph {
template<typename Key, typename Value>
using MapType = frozen::unordered_map<Key, Value>;
}
#else
# include <unordered_map>
# define CXXGraph_ConstexprMap
namespace CxxGraph {
template<typename Key, typename Value>
using MapType = std::unordered_map<Key, Value>;
}
#endif Benchmarks
|
I think the advantage in this moment does not justify the introduction of an external library. |
I agree, all the things I have been thinking about would be an opt in dependency rather than a hard requirement.
Yes, the user has to tell us what headers to include, what namespace the class is under and what the name of the class is. There are several ways I can think of to do the include:
For the namespace and class:
I doubt frozen will have a much higher overhead. To find out for sure I would have to write memory benchmarks for frozen to find out (the ones above are from their repo), as they use google benchmark this doesn't seem difficult, someone here has written something that could easily be added but I am not going to follow through with that as I think it's somewhat irrelevant for an optional dependency. If I get some time I will update the branch and implement 1 then mark the PR as ready |
@JustCallMeRay thank you so much! I Agree with you! |
c0cf7ad
to
ee6214c
Compare
…expr pre C++20 in most cases
…or using a constexpr set lib
ee6214c
to
4faf229
Compare
I have added constexpr specifiers to Node and Edge classes. These allow for the specified functions and objects to be ran at compile time instead of runtime, resulting in a speed increase.
A possible use case that might result in a speed up:
I have changed the UserId to be able to take anything as a template specialisation which may have a possible pain point with
const char*
being picked over strings and therefor doing a shallow copy over a deep string copy, so I have a static assert to make sure that never happens.