-
Notifications
You must be signed in to change notification settings - Fork 216
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
Various minor improvements #1202
Various minor improvements #1202
Conversation
@@ -27,6 +27,7 @@ | |||
#include <boost/range/begin.hpp> | |||
#include <boost/range/end.hpp> | |||
#include <boost/range/value_type.hpp> | |||
#include <boost/range/size.hpp> |
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.
After updating Boost to the most recent develop on my notebook (because of swap), this missing include was problematic for some unit tests, which is the reason why I checked all usages.
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.
indeed, this is caught by github actions too, which is still failing after the modifications of this PR.
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! I am OK in general but I think it's worth it to fix the missing header issue.
39fb0ab
to
30d26fe
Compare
For me it did not fail. But I added the include there too and it should now be fine. Thank you. |
30d26fe
to
9f9b24f
Compare
Now all tests pass. @vissarion could you approve? |
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.
Sure, thanks!
Fixes issue #1169
Adds unit tests for issue #1138 which was already fixed
Adds unit test for issue #1103 which was already fixed
Adds comments for epsilon as suggested by @vissarion
And some stylistic issues (const placements, includes)