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

force compiler warning if callback handles not captured #2000

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

jasonbeach
Copy link
Contributor

@jasonbeach jasonbeach commented Aug 20, 2022

force a compiler warning if callback handles are not captured.
clarify documentation to show that not capturing returned handles
will result in callbacks immediately being unregistered
see issue #1998
Signed-off-by: Jason Beach

@jasonbeach jasonbeach force-pushed the rolling branch 2 times, most recently from 531ca58 to 992eab1 Compare August 20, 2022 05:09
@jasonbeach
Copy link
Contributor Author

Uncrustify seems to be complaining about something, but I can't figure out what or how to fix it. Guidance would be appreciated.

@alsora
Copy link
Collaborator

alsora commented Aug 20, 2022

PR looks good to me.

The uncrustify output is the following

include/rclcpp/parameter_event_handler.hpp
+++ include/rclcpp/parameter_event_handler.hpp.uncrustify
@@ -89,2 +89,2 @@
- * Note: the object returned from add_parameter_callback must be captured or the callback will 
- * be immediately unregistered. 
+ * Note: the object returned from add_parameter_callback must be captured or the callback will
+ * be immediately unregistered.
@@ -159 +159 @@
- * the callback will immediately be unregistered. 
+ * the callback will immediately be unregistered.
@@ -198,2 +198,2 @@
-   * Note: if the returned callback handle smart pointer is not captured, the callback is 
-   * immediatedly unregistered. A compiler warning should be generated to warn of this. 
+   * Note: if the returned callback handle smart pointer is not captured, the callback is
+   * immediatedly unregistered. A compiler warning should be generated to warn of this.
@@ -227 +227 @@
-   * of this. 
+   * of this.

The problem is that you have trailing whitespaces at the end of the comments.
Make sure that lines don't end in a whitespace.
P. S. Try highlithing the uncrustify log and you will see that the current lines have a whitespace, while the suggested ones do not.. Unfortunately it's not evident unless you highlight it or paste it in an editor that shows whitespaces

clarify documentation to show that not capturing returned handles
will result in callbacks immediately being unregistered

Signed-off-by: Jason Beach <jason.m.beach@gmail.com>
@jasonbeach
Copy link
Contributor Author

what's the typical workflow you follow for using uncrustify? I normally use clangd and for that there's a .clangd / .clang-format / .clang-tidy file in the top level of the repo. For uncrustify, it looks like it takes a .cfg file to define the formatting, but I couldn't find one in the repo.

@alsora
Copy link
Collaborator

alsora commented Aug 20, 2022

Ros projects like this do not invoke uncrustify directly, but rather they use the "ament_uncrustify" wrapper.
Among the other features, this wrapper comes with a configuration file https://github.com/ament/ament_lint/blob/master/ament_uncrustify/ament_uncrustify/configuration/ament_code_style.cfg

@jasonbeach
Copy link
Contributor Author

all issues fixed. thanks.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Collaborator

CI:

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

@clalancette
Copy link
Contributor

This was approved and has green CI, so I'm going ahead and merging it.

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.

None yet

4 participants