Skip to content

Patch octomap to fix C++17 builds using libc++#11627

Merged
jamiesnape merged 1 commit intoRobotLocomotion:masterfrom
jamiesnape:patch-octomap-libcxx-cxx17
Jun 13, 2019
Merged

Patch octomap to fix C++17 builds using libc++#11627
jamiesnape merged 1 commit intoRobotLocomotion:masterfrom
jamiesnape:patch-octomap-libcxx-cxx17

Conversation

@jamiesnape
Copy link
Copy Markdown
Contributor

@jamiesnape jamiesnape commented Jun 12, 2019

Applies OctoMap/octomap#233 for C++17 compatibility.


This change is Reviewable

@jamiesnape
Copy link
Copy Markdown
Contributor Author

I cannot see this merging into OctoMap anytime soon, so +@jwnimmer-tri for feature review.

Copy link
Copy Markdown
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: feature; please test in macOS CI to err on the safe side.

Reviewed 5 of 5 files at r1.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jamiesnape)


tools/workspace/octomap/do_not_use_random_shuffle.patch, line 18 at r1 (raw file):

 #if defined(_MSC_VER) || defined(_LIBCPP_VERSION)
   #include <algorithm>
+  #if __cplusplus > 199711L

nit I would have expected >= 201103L here instead? I guess it doesn't matter in practice though.

@jamiesnape
Copy link
Copy Markdown
Contributor Author

@drake-jenkins-bot mac-mojave-clang-bazel-experimental-cxx17-release please

@jamiesnape jamiesnape requested a review from jwnimmer-tri June 13, 2019 16:36
@jamiesnape
Copy link
Copy Markdown
Contributor Author

+@ggould-tri for platform review.

@jamiesnape jamiesnape removed the request for review from jwnimmer-tri June 13, 2019 16:38
Copy link
Copy Markdown
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee ggould-tri(platform), labeled "do not merge" (waiting on @ggould-tri)

@jamiesnape
Copy link
Copy Markdown
Contributor Author

(FYI octomap has already successfully built at this stage of mac-mojave-clang-bazel-experimental-cxx17-release)

Copy link
Copy Markdown
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),jwnimmer-tri(platform)

@jamiesnape jamiesnape merged commit 8b492da into RobotLocomotion:master Jun 13, 2019
@jamiesnape jamiesnape deleted the patch-octomap-libcxx-cxx17 branch June 13, 2019 19:33
antequ pushed a commit to antequ-TRI/drake that referenced this pull request Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants