migrate to <random> #195
migrate to <random> #195
Conversation
rand/srand were probably already deprecated by the time this code was written...
I took the error on MoveIt's CI as an incentive to migrate to std::random:
As example code referred to a seeding step, I added an explicit setRandomSeed to the API.
<<<
rviz_visual_tools.cpp:2810:42: error: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Werror,-Wimplicit-int-float-conversion]
float d = static_cast<float>(rand()) / RAND_MAX;
~ ^~~~~~~~
/usr/include/stdlib.h:86:18: note: expanded from macro 'RAND_MAX'
^~~~~~~~~~
<<<
| @@ -1144,6 +1152,8 @@ class RvizVisualTools | |||
| // Chose random colors from this list | |||
| static const std::array<colors, 14> ALL_RAND_COLORS; | |||
|
|
|||
| static std::mt19937 mt_random_engine_; | |||
There was a problem hiding this comment.
because dRand, fRand, and iRand are and I did not want to break API by removing static from them.
There was a problem hiding this comment.
if at least one other team member agrees, I'm also open to change them to non-static members, but that might entail more changes in other places/repos
There was a problem hiding this comment.
I'm going to merge this out of expedience to get moveit CI working again, if we want to a further refactoring of this later we can.
There was a problem hiding this comment.
because
dRand,fRand, andiRandare and I did not want to break API by removing static from them.
Oh, interesting. And you mean ABI?
There was a problem hiding this comment.
ABI?
No, API.
RvizVisualTools::iRand(0,1) would not compile anymore if we remove static.
There was a problem hiding this comment.
o. I was asking about mt_random_engine_, not iRand
There was a problem hiding this comment.
yes, but how would you want to access a non-static random engine from the static iRand?
❓ |
|
Don't worry about it. There was some comma spacing I was going to ask about but if the linter is fine with it then I don't care. |
rand/srand were probably already deprecated by the time this code was written...
I took the error on MoveIt's CI as an incentive to migrate to std::random:
As example code referred to a seeding step, I added an explicit setRandomSeed to the API.
This is required to fix MoveIt's CI: moveit/moveit@2989f44
<<<
rviz_visual_tools.cpp:2810:42: error: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Werror,-Wimplicit-int-float-conversion]
float d = static_cast(rand()) / RAND_MAX;
~ ^~~~~~~~
/usr/include/stdlib.h:86:18: note: expanded from macro 'RAND_MAX'
^~~~~~~~~~
<<<
Please review @nbbrooks @tylerjw @JafarAbdi