Skip to content
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

Add missing extern "C", RCL_WARN_UNUSED #650

Closed
wants to merge 1 commit into from
Closed

Conversation

rotu
Copy link
Contributor

@rotu rotu commented May 15, 2020

No description provided.

@dirk-thomas
Copy link
Member

Thanks for the patch.

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

@rotu
Copy link
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
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
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
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
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