Skip to content

Add missing extern "C", RCL_WARN_UNUSED#650

Closed
rotu wants to merge 1 commit intoros2:masterfrom
rotu:patch-1
Closed

Add missing extern "C", RCL_WARN_UNUSED#650
rotu wants to merge 1 commit intoros2:masterfrom
rotu:patch-1

Conversation

@rotu
Copy link
Copy Markdown
Contributor

@rotu rotu commented May 15, 2020

No description provided.

Signed-off-by: Dan Rose <dan@digilabs.io>
@dirk-thomas
Copy link
Copy Markdown
Member

Thanks for the patch.

Please run the tests locally before proposing changes and address the test failures.

@rotu
Copy link
Copy Markdown
Contributor Author

rotu commented May 15, 2020

I made these code changes through the GitHub web interfaces, so running tests locally first wasn't an option.

At any rate, closing this PR, since I didn't realize the file is duplicated here https://github.com/ros2/rcl_logging/blob/8ce282501695af7144e761d0fa3bd4ad56d53974/rcl_logging_log4cxx/include/rcl_logging_log4cxx/logging_interface.h

@rotu rotu closed this May 15, 2020
@dirk-thomas
Copy link
Copy Markdown
Member

I made these code changes through the GitHub web interfaces, so running tests locally first wasn't an option.

@rotu If you make pull requests in the future without compiling / running / testing them locally please state that explicitly in the description since it is expected that the author does that and reviewers won't expect the opposite. Thanks.

@rotu
Copy link
Copy Markdown
Contributor Author

rotu commented May 15, 2020

@dirk-thomas That's exactly the low-hanging fruit that CI is meant to catch, and in this case, it did its job! I try to make PRs that are not yet review-ready drafts instead of putting it in the description.

@dirk-thomas
Copy link
Copy Markdown
Member

That's exactly the low-hanging fruit that CI is meant to catch, and in this case, it did its job!

The CI system doesn't exist to let the developer skip their due diligence and test their patches before contributing them.

@rotu
Copy link
Copy Markdown
Contributor Author

rotu commented May 15, 2020

Generally true. In this case, it's all compilation-time changes. So yes, any undesired side effects would be caught by CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants