-
Notifications
You must be signed in to change notification settings - Fork 947
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
[windows][master] some more header inclusion and MSVC build error fixes. #1636
Conversation
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.
Overall this looks good, but please have a look at my comments.
Thank you so much for taking the time to clean up such micro-issues.
moveit_planners/chomp/chomp_motion_planner/src/chomp_optimizer.cpp
Outdated
Show resolved
Hide resolved
@v4hn Thanks for the review. I updated the pull requests to address some feedback. |
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.
Looks good to me.
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.
Most of these changes make sense to me. Please have a look at my comments.
On Thu, Aug 22, 2019 at 11:05:37AM -0700, Robert Haschke wrote:
My key point is to avoid defines.
You do not avoid them, just move them to a compatibility header in (I guess) `moveit_core/compat`. ;)
1. rely on the developer to include them before the compat header when necessary
:/
2. or explicitly declare those required functions in the compat header.
This could be an option, although I can imagine different Unix standards might conflict here. ;-D
In either case you will still need `#ifdef`'s to distinguish between the includes.
So I do not see the merit of avoiding 1-3 defines in addition to the guarded includes.
|
Indeed, I would prefer this over `std::make_pair(static_cast<int>(index),0)`.
However, I'm not sure about the actual problem in Windows. Maybe `std::make_pair<int>()` is just not available as I assume...
I fear we might end up with `std::make_pair<int, int>(static_cast<int>(index), 0)` to address your concerns *and* fix the windows compiler issue. ;)
Let's wait for feedback from the author.
|
To be clear about this: In the code base we would just do: |
However, using the second option, there is nothing more to do.
This is true as long as you do not want to have compile-time (vs. link time) validation
that the specified function signature is actually correct for your system.
Usually people still include the host's headers to get error messages on signature mismatch.
But I guess we might be able to skip this.
|
Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>
Thanks for your patience 😉 |
Thanks @seanyen 👍 |
…eit#1636) * Fix header inclusion for Windows build. * replace random() with c++11 <random> usage.
…eit#1636) * Fix header inclusion for Windows build. * replace random() with c++11 <random> usage.
static_cast
forindex
inplan_execution::PlanExecution::successfulTrajectorySegmentExecution
to avoid MSVC compiler error.const
forSortBodies::operator()
to avoid C3848 error by MSVC.<random>
for portable random number generation.