[POC] Apply default GSG-based ROS2 format #872

hidmic · 2019-09-24T22:05:44Z

This pull request reformats rclcpp using ament_clang_format with the configuration in this patch. It attempts to minimize the diff to comply with the linter.

It remains to be confirmed whether ament_uncrustify configuration can be updated to accept these changes.

Connected to ament/ament_lint#146.

Signed-off-by: Michel Hidalgo <[email protected]>

wjwwood

I started this review a few nights ago, but didn't finish it, but here are the notes I made along the way. They're just my opinion and some are aspirational I guess.

wjwwood · 2019-09-24T22:11:58Z

rclcpp/include/rclcpp/any_service_callback.hpp

+  using SharedPtrWithRequestHeaderCallback = std::function<void(
+    const std::shared_ptr<rmw_request_id_t>,
+    const std::shared_ptr<typename ServiceT::Request>,
+    std::shared_ptr<typename ServiceT::Response>)>;


This is an example of something that, while still readable after applying clang-format, is not better in my opinion. I think the original author probably picked the style so it was most readable to them (within some basic formatting rules like max line length and wrapping style).

If a pr came in with the new style I wouldn't ask them to change it (unless it was particularly tricky to read in some case), but I do think that letting the developer have some autonomy to decide how it looks best is super important. That's my main concern with clang-format, as it doesn't have much flexibility for cases like this.

Right, I can confirm clang-format won't give a developer much freedom of choice.

wjwwood · 2019-09-24T22:15:38Z

rclcpp/include/rclcpp/any_service_callback.hpp


  SharedPtrCallback shared_ptr_callback_;
  SharedPtrWithRequestHeaderCallback shared_ptr_with_request_header_callback_;

 public:
  AnyServiceCallback()
  : shared_ptr_callback_(nullptr), shared_ptr_with_request_header_callback_(nullptr)
-  {}
+  {
+  }


Another case where both are perfectly valid ways to write the code (while I see minor value in one over the other, neither are unreadable), but since the developer has some freedom here they can save some vertical space in a case where using more doesn't help.

However, If they think it does help for some reason (maybe it makes it more consistent with other functions in the same file that have single statement bodies, or maybe it will reduce the diff in the future), then them having the flexibility to make that choice (and defend it if needed, to a reviewer who questions it) is very important in my opinion.

then them having the flexibility to make that choice (and defend it if needed, to a reviewer who questions it) is very important in my opinion.

IMHO this is key to this entire discussion.

It doesn't seem to me like clang-format ever intended to provide the grounds for such exchanges to happen. There's a set of rules that code must comply with, period. Authors do not have a saying in this regard, neither do reviewers. And I don't think that's bad on its own, specially as a code base grows large. It keeps our own personal biases out the door e.g. when reviewing you may think X style variation (within permissible boundaries) is fine while I may not, and viceversa.

Now, that may not be ideal nor appropriate for ROS2. If it's not, then maybe clang-format is not a good choice. We know uncrustify can be configured to be much more tolerant -- bugs aside.

wjwwood · 2019-09-24T22:17:56Z

rclcpp/include/rclcpp/any_service_callback.hpp

-    >::type * = nullptr
-  >
+      rclcpp::function_traits::same_arguments<CallbackT, SharedPtrCallback>::value>::type * =
+      nullptr>


This is a case where I think the clang-format way is less readable (my personal opinion obviously), and that the previous way was actually a lot better, even though it takes up more vertical space.

I agree on this one too.

wjwwood · 2019-09-24T22:21:10Z

rclcpp/include/rclcpp/any_subscription_callback.hpp

-                "unexpected dispatch_intra_process const shared "
-                "message call with no const shared_ptr callback");
+          "unexpected dispatch_intra_process const shared "
+          "message call with no const shared_ptr callback");


This is a case where clang-format is better, but I think there's an on-going issue about addressing this in uncrustify.

However, it's a case where I just don't care. It's a bit unsightly, but it doesn't harm readability significantly, in my opinion. These are the kind of linter shortcomings that are annoying but not consequential in my opinion.

wjwwood · 2019-09-24T22:23:53Z

rclcpp/include/rclcpp/callback_group.hpp

-  MutuallyExclusive,
-  Reentrant
-};
+enum class CallbackGroupType { MutuallyExclusive, Reentrant };


I think this is a step backwards too. Neither is clearly better, but if a developer picked one over the other, I think we should leave it until we need to add or remove something from the enum.

Also, a purely subjective comment is that, I really don't like space around the items in the {}, you would never do this in Python for example, you'd do {'foo': bar, 'ping': pong} for instance.

Arguably, if types are added/removed the diff would be easier to read with the multi-line format.

wjwwood · 2019-09-24T22:32:18Z

rclcpp/include/rclcpp/client.hpp

+      auto response = future.get();
+      promise->set_value(std::make_pair(request, response));
+      cb(future_with_request);
+    };


This is a strict improvement for sure. But I think this is a bug in uncrustify.

Again this is annoying, but not something worth losing time over in my opinion.

wjwwood · 2019-09-24T22:32:40Z

rclcpp/include/rclcpp/clock.hpp

-  using pre_callback_t = std::function<void ()>;
-  using post_callback_t = std::function<void (const rcl_time_jump_t &)>;
+  using pre_callback_t = std::function<void()>;
+  using post_callback_t = std::function<void(const rcl_time_jump_t &)>;


I don't fine this easier to read, personally.

wjwwood · 2019-09-24T22:33:38Z

rclcpp/include/rclcpp/create_service.hpp

@@ -46,8 +45,7 @@ create_service(
  service_options.qos = qos_profile;

  auto serv = Service<ServiceT>::make_shared(
-    node_base->get_shared_rcl_node_handle(),
-    service_name, any_service_callback, service_options);
+    node_base->get_shared_rcl_node_handle(), service_name, any_service_callback, service_options);


This is another case where it seems the developer thought this was a more reasonable way to break these across lines and in my opinion the linter should have left it alone.

wjwwood · 2019-09-24T22:34:18Z

rclcpp/include/rclcpp/create_subscription.hpp

@@ -40,21 +40,18 @@ template<
  typename CallbackT,
  typename AllocatorT = std::allocator<void>,
  typename CallbackMessageT =
-  typename rclcpp::subscription_traits::has_message_type<CallbackT>::type,
+    typename rclcpp::subscription_traits::has_message_type<CallbackT>::type,


This is a nice improvement. Again, a bug in uncrustify in my opinion.

wjwwood · 2019-09-24T22:38:07Z

rclcpp/include/rclcpp/create_subscription.hpp

-    std::forward<CallbackT>(callback), options.event_callbacks, msg_mem_strat, allocator);
+  auto factory = rclcpp::
+    create_subscription_factory<MessageT, CallbackT, AllocatorT, CallbackMessageT, SubscriptionT>(
+      std::forward<CallbackT>(callback), options.event_callbacks, msg_mem_strat, allocator);


I wouldn't have picked either of these, I would have done:

auto factory = rclcpp::create_subscription_factory< MessageT, CallbackT, AllocatorT, CallbackMessageT, SubscriptionT >( std::forward<CallbackT>(callback), options.event_callbacks, msg_mem_strat, allocator ); // or auto factory = rclcpp::create_subscription_factory< MessageT, CallbackT, AllocatorT, CallbackMessageT, SubscriptionT>( std::forward<CallbackT>(callback), options.event_callbacks, msg_mem_strat, allocator); // or even better, get rid of the redundant template arguments (as we did in master already) auto factory = rclcpp::create_subscription_factory<MessageT>( std::forward<CallbackT>(callback), options.event_callbacks, msg_mem_strat, allocator);

But I would let the author decide what they like better. As long as it followed the guidelines, like max line length and avoiding partially wrapping arguments (some on the same line as opening scope, some not).

ivanpauno · 2020-04-29T17:51:57Z

I think we can close this PR, until we have settled if we want to go ahead with the changes in ament/ament_lint#146.

* Add const keyword to signature of rcl_context_{get_instance_id,is_valid} Signed-off-by: Thijs Raymakers <[email protected]>

Apply default GSG-based ROS2 format.

b70aabb

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic mentioned this pull request Sep 24, 2019

ament_uncrustify and ament_clang_format cannot both be satisfied ament/ament_lint#146

Closed

wjwwood reviewed Oct 2, 2019

View reviewed changes

mabelzhang added the in progress Actively being worked on (Kanban column) label Oct 3, 2019

ivanpauno closed this Apr 29, 2020

clalancette deleted the hidmic/clang-format branch January 15, 2021 16:32

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Add const to constant rcl_context functions (ros2#872)

e569f3f

* Add const keyword to signature of rcl_context_{get_instance_id,is_valid} Signed-off-by: Thijs Raymakers <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Apply default GSG-based ROS2 format #872

[POC] Apply default GSG-based ROS2 format #872

hidmic commented Sep 24, 2019 •

edited

Loading

wjwwood left a comment

wjwwood Sep 24, 2019

hidmic Oct 2, 2019

wjwwood Sep 24, 2019

hidmic Oct 2, 2019

wjwwood Sep 24, 2019

hidmic Oct 2, 2019

wjwwood Sep 24, 2019

wjwwood Sep 24, 2019

jacobperron Oct 10, 2019 •

edited

Loading

wjwwood Sep 24, 2019

wjwwood Sep 24, 2019

wjwwood Sep 24, 2019

wjwwood Sep 24, 2019

wjwwood Sep 24, 2019

ivanpauno commented Apr 29, 2020

[POC] Apply default GSG-based ROS2 format #872

[POC] Apply default GSG-based ROS2 format #872

Conversation

hidmic commented Sep 24, 2019 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpauno commented Apr 29, 2020

hidmic commented Sep 24, 2019 •

edited

Loading

jacobperron Oct 10, 2019 •

edited

Loading