Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Enable basic warnings#22

Closed
audrow wants to merge 7 commits intoros2from
audrow/enable-warnings-urdfdom
Closed

Enable basic warnings#22
audrow wants to merge 7 commits intoros2from
audrow/enable-warnings-urdfdom

Conversation

@audrow
Copy link
Copy Markdown
Member

@audrow audrow commented Sep 22, 2020

No description provided.

@audrow audrow self-assigned this Sep 22, 2020
clalancette
clalancette suggested changes Sep 22, 2020
View reviewed changes
urdf_parser/test/memtest.cpp Outdated
#include <iostream>

int main(int argc, char** argv){
(void) argc;
Copy link
Copy Markdown

@clalancette clalancette Sep 22, 2020

Choose a reason for hiding this comment

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

Actually, rather than doing this, I think we should fix this main up, since it is buggy.

That is, if it ever got called with no arguments, it would index past the end of argv below and crash.

My suggestion here is to add:

if (argc != 2) {
  fprintf(stderr, "Usage: %s <urdf>\n", argv[0]);
  return 1;
}

And also add a return 0; to the end of the function for completeness.

@audrow audrow requested a review from clalancette September 22, 2020 21:59
clalancette
clalancette approved these changes Sep 22, 2020
View reviewed changes
Copy link
Copy Markdown

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good with green CI.

@audrow
Copy link
Copy Markdown
Member Author

audrow commented Sep 23, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@audrow
Copy link
Copy Markdown
Member Author

audrow commented Sep 24, 2020

For the MacOS CI, it seems that including console_bridge/console.h, includes a file that uses variadic macros.

ros/console_bridge#64 seems to be the solution to this problem. It was merged-in in May before the console_bridge 1.0.1 tag, which I believe is the one the used in the current console_bridge_vendor (here). However, the error suggests that ros/console_bridge#64 hasn't been applied, as it is the same warning addressed in the PR (note the ##__VA_ARGS).

[  3%] Building CXX object urdf_parser/CMakeFiles/urdfdom_model.dir/src/pose.cpp.o
In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/src/ros2/urdfdom/urdf_parser/src/pose.cpp:42:
/usr/local/include/console_bridge/console.h:67:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)

@audrow audrow mentioned this pull request Sep 25, 2020
Closed
@audrow
Copy link
Copy Markdown
Member Author

audrow commented Oct 6, 2020 •
edited
Loading

Here is a rerun of CI on macOS with a newer version of googletest (v1.10.0) from this PR: ament/googletest#8.
* macOS Build Status

Actually, this is misleading because the issue with macOS in this case is that the wrong headers are being applied.

@audrow audrow mentioned this pull request Oct 6, 2020
Merged
audrow added 6 commits October 15, 2020 11:56
@audrow
Enable basic warnings
7da6841 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Handle input arguments and return in main function
15fea17 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Set default C compiler
26ab3ee 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Add extern "C" around console_bridge/console.h include
c2c5b67 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Revert "Add extern "C" around console_bridge/console.h include"
c0501fd 
This reverts commit 9a4d55d.
@audrow
Revert "Set default C compiler"
c67038a 
This reverts commit 542110f.
@audrow audrow force-pushed the audrow/enable-warnings-urdfdom branch from 0a917ab to c67038a Compare October 15, 2020 18:57
@clalancette
Use SYSTEM for the include directories in urdfdom.
40188c7 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@audrow
Copy link
Copy Markdown
Member Author

audrow commented Oct 21, 2020

Testing urdfdom and packages above it:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Copy Markdown

clalancette commented Oct 27, 2020

@audrow So, I've now moved main ROS 2 development for this package over to https://github.com/ros/urdfdom. Could you please move this PR over to that repository and close this one out? Thanks.

This was referenced Oct 27, 2020
Closed
Merged
@audrow
Copy link
Copy Markdown
Member Author

audrow commented Oct 27, 2020

I've moved this PR over to ros#148.

@audrow audrow closed this Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@clalancette clalancette clalancette approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@audrow audrow

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@audrow @clalancette