-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor numeric_cast #1
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
Refactor numeric_cast #1
Conversation
This allows to extend numeric_cast to other argument types than mp_integer.
This returns a value instead of an optional but an invariant can fail.
numeric_cast can now be used instead.
This is to demonstrate the use of numeric_cast
std::numeric_limits methods are not declared constexpr in old versions of VS so we need to replace them by preconditions
|
|
||
| optionalt<T> operator()(const mp_integer &mpi) const | ||
| { | ||
| constexpr const auto precondition = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely this will be a static error on VS2013 which doesn't have constexprs in the standard library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, potentially yeah. You could put an ifdef around the constexpr I suppose, or we might have a macro that conditionally expands to constexpr in util somewhere.
| static optionalt<T> numeric_cast(const mp_integer &mpi) | ||
| template <typename U = T, | ||
| typename std::enable_if<std::is_signed<U>::value, int>::type = 0> | ||
| static auto get_val(const mp_integer &mpi) -> decltype(mpi.to_long()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should both be private
romainbrenguier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal. What I don't find nice is that we pull a lot of things from arith_tools to mp_arith which I think we want to keep seperate. Could we instead define these numeric_cast things in a new file?
|
|
||
| const mp_integer mp_zero=string2integer("0"); | ||
|
|
||
| class exprt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate of line 23?
|
The problem is that I think all the numeric cast overloads need to be defined in one go. So as long as all the numeric_cast definitions are moved, that will work, but moving just a few of them won't |
d69a2dd to
7fbfeec
Compare
|
I'm not sure what to do with this branch. I don't plan to work on it any more, as all the suggested changes are very minor. I'd suggest that you cherry-pick any changes you like (if any) and close it once you've extracted anything useful. |
Yes no problem, I already integrated some of the changes in my branch. Just leave this open in case there are other changes I could use. |
70598c4 to
9a811d8
Compare
We should keep the struct tag type, not follow it and get the struct type
No description provided.