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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion rclcpp/include/rclcpp/allocator/allocator_common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_al
return std::allocator_traits<Alloc>::allocate(*typed_allocator, size);
}


// Convert a std::allocator_traits-formatted Allocator into an rcl allocator
template<
typename T,
Expand Down
22 changes: 10 additions & 12 deletions rclcpp/include/rclcpp/allocator/allocator_deleter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,11 @@ class AllocatorDeleter
using AllocRebind = typename std::allocator_traits<Allocator>::template rebind_alloc<T>;

public:
AllocatorDeleter()
: allocator_(nullptr)
AllocatorDeleter() : allocator_(nullptr)
{
}

explicit AllocatorDeleter(Allocator * a)
: allocator_(a)
explicit AllocatorDeleter(Allocator * a) : allocator_(a)
{
}

Expand Down Expand Up @@ -71,16 +69,16 @@ class AllocatorDeleter
template<typename Alloc, typename T, typename D>
void set_allocator_for_deleter(D * deleter, Alloc * alloc)
{
(void) alloc;
(void) deleter;
(void)alloc;
(void)deleter;
throw std::runtime_error("Reached unexpected template specialization");
}

template<typename T, typename U>
void set_allocator_for_deleter(std::default_delete<T> * deleter, std::allocator<U> * alloc)
{
(void) deleter;
(void) alloc;
(void)deleter;
(void)alloc;
}

template<typename Alloc, typename T>
Expand All @@ -94,11 +92,11 @@ void set_allocator_for_deleter(AllocatorDeleter<T> * deleter, Alloc * alloc)

template<typename Alloc, typename T>
using Deleter = typename std::conditional<
std::is_same<typename std::allocator_traits<Alloc>::template rebind_alloc<T>,
typename std::allocator<void>::template rebind<T>::other>::value,
std::is_same<
typename std::allocator_traits<Alloc>::template rebind_alloc<T>,
typename std::allocator<void>::template rebind<T>::other>::value,
std::default_delete<T>,
AllocatorDeleter<Alloc>
>::type;
AllocatorDeleter<Alloc>>::type;
} // namespace allocator
} // namespace rclcpp

Expand Down
39 changes: 14 additions & 25 deletions rclcpp/include/rclcpp/any_service_callback.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,51 +31,40 @@ template<typename ServiceT>
class AnyServiceCallback
{
private:
using SharedPtrCallback = std::function<
void (
const std::shared_ptr<typename ServiceT::Request>,
std::shared_ptr<typename ServiceT::Response>
)>;
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>
)>;
using SharedPtrCallback = std::function<void(
const std::shared_ptr<typename ServiceT::Request>,
std::shared_ptr<typename ServiceT::Response>)>;
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>)>;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


SharedPtrCallback shared_ptr_callback_;
SharedPtrWithRequestHeaderCallback shared_ptr_with_request_header_callback_;

public:
AnyServiceCallback()
: shared_ptr_callback_(nullptr), shared_ptr_with_request_header_callback_(nullptr)
{}
{
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


AnyServiceCallback(const AnyServiceCallback &) = default;

template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
SharedPtrCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, SharedPtrCallback>::value>::type * =
nullptr>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on this one too.

void set(CallbackT callback)
{
shared_ptr_callback_ = callback;
}

template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
SharedPtrWithRequestHeaderCallback
>::value
>::type * = nullptr
>
typename std::enable_if<rclcpp::function_traits::same_arguments<
CallbackT,
SharedPtrWithRequestHeaderCallback>::value>::type * = nullptr>
void set(CallbackT callback)
{
shared_ptr_with_request_header_callback_ = callback;
Expand Down
88 changes: 32 additions & 56 deletions rclcpp/include/rclcpp/any_subscription_callback.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ class AnySubscriptionCallback
using ConstMessageSharedPtr = std::shared_ptr<const MessageT>;
using MessageUniquePtr = std::unique_ptr<MessageT, MessageDeleter>;

using SharedPtrCallback = std::function<void (const std::shared_ptr<MessageT>)>;
using SharedPtrCallback = std::function<void(const std::shared_ptr<MessageT>)>;
using SharedPtrWithInfoCallback =
std::function<void (const std::shared_ptr<MessageT>, const rmw_message_info_t &)>;
using ConstSharedPtrCallback = std::function<void (const std::shared_ptr<const MessageT>)>;
std::function<void(const std::shared_ptr<MessageT>, const rmw_message_info_t &)>;
using ConstSharedPtrCallback = std::function<void(const std::shared_ptr<const MessageT>)>;
using ConstSharedPtrWithInfoCallback =
std::function<void (const std::shared_ptr<const MessageT>, const rmw_message_info_t &)>;
using UniquePtrCallback = std::function<void (MessageUniquePtr)>;
std::function<void(const std::shared_ptr<const MessageT>, const rmw_message_info_t &)>;
using UniquePtrCallback = std::function<void(MessageUniquePtr)>;
using UniquePtrWithInfoCallback =
std::function<void (MessageUniquePtr, const rmw_message_info_t &)>;
std::function<void(MessageUniquePtr, const rmw_message_info_t &)>;

SharedPtrCallback shared_ptr_callback_;
SharedPtrWithInfoCallback shared_ptr_with_info_callback_;
Expand All @@ -58,9 +58,12 @@ class AnySubscriptionCallback

public:
explicit AnySubscriptionCallback(std::shared_ptr<Alloc> allocator)
: shared_ptr_callback_(nullptr), shared_ptr_with_info_callback_(nullptr),
const_shared_ptr_callback_(nullptr), const_shared_ptr_with_info_callback_(nullptr),
unique_ptr_callback_(nullptr), unique_ptr_with_info_callback_(nullptr)
: shared_ptr_callback_(nullptr),
shared_ptr_with_info_callback_(nullptr),
const_shared_ptr_callback_(nullptr),
const_shared_ptr_with_info_callback_(nullptr),
unique_ptr_callback_(nullptr),
unique_ptr_with_info_callback_(nullptr)
{
message_allocator_ = std::make_shared<MessageAlloc>(*allocator.get());
allocator::set_allocator_for_deleter(&message_deleter_, message_allocator_.get());
Expand All @@ -71,12 +74,8 @@ class AnySubscriptionCallback
template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
SharedPtrCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, SharedPtrCallback>::value>::type * =
nullptr>
void set(CallbackT callback)
{
shared_ptr_callback_ = callback;
Expand All @@ -85,12 +84,8 @@ class AnySubscriptionCallback
template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
SharedPtrWithInfoCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, SharedPtrWithInfoCallback>::value>::
type * = nullptr>
void set(CallbackT callback)
{
shared_ptr_with_info_callback_ = callback;
Expand All @@ -99,12 +94,8 @@ class AnySubscriptionCallback
template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
ConstSharedPtrCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, ConstSharedPtrCallback>::value>::type * =
nullptr>
void set(CallbackT callback)
{
const_shared_ptr_callback_ = callback;
Expand All @@ -113,12 +104,8 @@ class AnySubscriptionCallback
template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
ConstSharedPtrWithInfoCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, ConstSharedPtrWithInfoCallback>::value>::
type * = nullptr>
void set(CallbackT callback)
{
const_shared_ptr_with_info_callback_ = callback;
Expand All @@ -127,12 +114,8 @@ class AnySubscriptionCallback
template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
UniquePtrCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, UniquePtrCallback>::value>::type * =
nullptr>
void set(CallbackT callback)
{
unique_ptr_callback_ = callback;
Expand All @@ -141,19 +124,14 @@ class AnySubscriptionCallback
template<
typename CallbackT,
typename std::enable_if<
rclcpp::function_traits::same_arguments<
CallbackT,
UniquePtrWithInfoCallback
>::value
>::type * = nullptr
>
rclcpp::function_traits::same_arguments<CallbackT, UniquePtrWithInfoCallback>::value>::
type * = nullptr>
void set(CallbackT callback)
{
unique_ptr_with_info_callback_ = callback;
}

void dispatch(
std::shared_ptr<MessageT> message, const rmw_message_info_t & message_info)
void dispatch(std::shared_ptr<MessageT> message, const rmw_message_info_t & message_info)
{
if (shared_ptr_callback_) {
shared_ptr_callback_(message);
Expand Down Expand Up @@ -185,20 +163,18 @@ class AnySubscriptionCallback
const_shared_ptr_with_info_callback_(message, message_info);
} else {
if (
unique_ptr_callback_ || unique_ptr_with_info_callback_ ||
shared_ptr_callback_ || shared_ptr_with_info_callback_)
{
unique_ptr_callback_ || unique_ptr_with_info_callback_ || shared_ptr_callback_ ||
shared_ptr_with_info_callback_) {
throw std::runtime_error(
"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");
Copy link
Member

Choose a reason for hiding this comment

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

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.

} else {
throw std::runtime_error("unexpected message without any callback set");
}
}
}

void dispatch_intra_process(
MessageUniquePtr message, const rmw_message_info_t & message_info)
void dispatch_intra_process(MessageUniquePtr message, const rmw_message_info_t & message_info)
{
if (shared_ptr_callback_) {
typename std::shared_ptr<MessageT> shared_message = std::move(message);
Expand All @@ -212,8 +188,8 @@ class AnySubscriptionCallback
unique_ptr_with_info_callback_(std::move(message), message_info);
} else if (const_shared_ptr_callback_ || const_shared_ptr_with_info_callback_) {
throw std::runtime_error(
"unexpected dispatch_intra_process unique message call"
" with const shared_ptr callback");
"unexpected dispatch_intra_process unique message call"
" with const shared_ptr callback");
} else {
throw std::runtime_error("unexpected message without any callback set");
}
Expand Down
Loading