-
Notifications
You must be signed in to change notification settings - Fork 107
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
ament_uncrustify and ament_clang_format cannot both be satisfied #146
Comments
@hidmic said (somewhere else, but I think this convo belongs here):
There are places where I thing things like |
@rotu thanks for reproducing my comment here! I agree with you in that Regarding the In any case, I personally think we should aim for readability rather than blind compliance with some style guide, which we can always change or ammend. |
Here's at least one case where it enforces bad style: #173 I don't think I'm going to agree with you on most of those rules. The Google style guide is clear that vertical whitespace does not help in the beginning of a function. I assume the same applies to other blocks, and all those rules are examples of giving up vertical whitespace for negligible gain.
👍 |
@rotu An unspecific statement like this isn't helping in this discussion. Please bring up specific cases (not pointing to a whole file and stating you hate it) if you want the feedback to be actionable.
@rotu The specific case is a bug in uncrustify - potentially being triggered by our current set of configuration options. The style does certainly not suggest to format the code like this. In other code parts where this is the case we even opted to disable uncrustify for these lines to keep the indentation as desired without a linter warning: https://github.com/ros2/rclcpp/blob/458967bb568df93d31dbff0e10d198b4a065db70/rclcpp/include/rclcpp/service.hpp#L168-L171 |
Thank you for calling me out on that. I specifically meant having the function specifier and return type on different lines from the function name, which is done throughout that source file:
Kind of. It's due to aligning continued statements to the open parenthesis, which is against our style guide. In the case of a throw statement, it aligns to a virtual parenthesis that uncrustify places after the |
👍 that is certainly not documented in the style guide and also against our general goal to minimize vertical space. So that is something I would agree we should reconsider / reformat.
Nobody is opposed to fix those and merge the PR #174. The acceptance criteria though is that afterwards CI still passes green - so existing code which needs to be changed must have a corresponding PR which can be tested together with #174 and pass all linters. Basically a set of PRs when merged together will transition us from the current state passing on CI to a new state (with better style) which also passed on CI. |
I know. I just haven't time to do this in the one fell swoop required, but I will eventually! I bring it up to say it's a bug in ament_uncrustify, not one in uncrustify. |
Please just keep changes minimal per set of PRs. So don't try to address other style issues in the same set. Otherwise chance are pretty high its gets delay due to difficulties reviewing it. |
Replicating #148 (comment). In an attempt to get Applying the existing
Given that settings that
Please, comment on which style variation of the ones that |
It is true that it is not documented in the style guide that you must do it this way, because you don't, but there's no problem with doing it either. On the point that we should consider reformatting because it goes against a "goal to minimize vertical space", I completely disagree. We don't have a stated goal to do that as far as I know. The Google Style guideline (it even points out it is not really a hard rule) referenced above (#146 (comment)) is specifically speaking about blank lines. It never mentions separating parts of a declaration as wasting vertical space. I would not approve any pull request that only makes this change to our style guide, linters, or the code itself. |
@hidmic can you make a pull request with the proposed changes to rclcpp (I realize there are options to choose from, and so there's no one set of changes, but...), because without a pull request we cannot make inline comments. |
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace
|
I think you should have included the full quote:
It also says:
Then it says:
Suggesting that use of vertical space may be useful if it aids in readability. The rule exists, in my opinion, to avoid things like this:
Not to prevent things like:
Or to separate function names and return types in order to have all the names start in the same column. |
I don't know how much more clear the Google C++ style guide can be on this point:
|
Also, while the GSG does say "no spaces inside empty braces", it does not say to not insert newlines in empty spaces. I think it is generally implied by the vertical whitespace section. |
You're right, it does say that, I was not aware of that line, and I was wrong when I suggested this wasn't violating the GSG. However, the other rule about vertical whitespace I think still doesn't apply. In that same section (https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions) it also shows how to handle functions that are too long, for example:
Which as a team, years ago, we decided we did not like for various reasons. This is why we have fairly detailed exceptions where we diverge from GSG on this point in our style guide: https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#examples Also, wrapping the lines as I am often likely to do, is allowed when the function declaration is too long. My personal preference is to have the methods use the same wrapping pattern (if even one is too long and needs to be wrapped, wrap them all), but I can see that it would be more in line with the GSG if we didn't leave how to wrap functions declarations up to the developer. I think, however, the point of this is issue (at least my understand from our most recent ROS 2 meeting) is whether or not we want to use clang-format to do linting, and not rehash all the ways we are diverging from GSG. We may need to change some things about our style to change to clang-format, and that might bring up closer to the GSG, but I don't think we need to get closer to GSG just for the sake of being closer to the GSG. |
I very much like the wrapping style of ROS2. I think it's nicer than packing as many arguments as will fit on the first line and letting them overflow. I didn't realize there was a meeting to discuss this and that's encouraging. Clang-format and clang-tidy are suuuper useful tools that not only create better code, but teach me how to be a better coder. I would like them to be the default, or at least compatible with the default settings of ament_lint_auto (i.e. ament_uncrustify). |
It came up at our most recent general ROS 2 meeting. We're looking into it, and we would generally like to switch to clang-format just for the network effect (so many people using it and better tool integration for it). But some of us aren't yet convinced that we can achieve the style we want with clang-format (that's what we're investigating now) and yet others are unconvinced that chaging anything is worth the effort and disruption (people maintaining patches on top of rclcpp would hate those diffs, for example). So no promises, but we're looking into it. As for clang-tidy, I think that could be applied without (whitespace) style changes, right? |
Yes. You can turn off any of the clang-tidy checks, but I think only this one check critiques whitespace (and should be on because it's hella useful): https://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html |
Are there any news about the current status of using/switching to clang-format or from the outcome of the meeting? |
I certainly would be in favor of updating In terms of switching to clang-format by default, I would be against that. We already have two C++ linters (cpplint and uncrustify), and I don't really see the advantage of adding/switching to another one. |
cpplint does not support auto formatting code, so it's not at all comparable to clang-format. clang-format allows to fully format your code without forcing you to follow any coding style which is always a nice thing and it's broadly used also by many well known projects like the Linux kernel, Firefox or MS Visual Studio. The support for common editors is also much better, Clang-Format in VS Code has more than 500.000 installs while Uncrustify in VS Code has just a bit more than 30.000 and also seems to be dead/deprecated. Also using clang-format has the advantage that it fully supports newer C++ standards as it is based on the clang compiler while uncrustify is complete independent and thus less proven and maybe slower at understanding new code from newer standards. |
Old code could be updated only when changed (there are tools like pre-commit) so that not the complete history is broken by a huge reformat of everything (even if it might make sense to do that so it's done within a single commit). static
rclcpp::QoS
get_parameter_events_qos( https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/graph_listener.cpp#L238 static bool
has_node_( https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/node_options.cpp#L93 const rcl_node_options_t *
NodeOptions::get_rcl_node_options() const Probably more that can be found but I just clicked through a few files... |
The original issue that was opened here was about We also can't switch to Finally, the suggestion to change our style is way too much work for way too little gain. We have a style that works OK, and we have a linter ( Because of all of that, and because there is nothing actionable to do here, I'm going to close this issue out. If you'd like to open a separate issue about changing the style, feel free to do so, but we are unlikely to do that without a very compelling reason. |
ament_uncrustify
andament_clang_format
have defaults which differ in a way that you can't satisfy both at the same time. What's worse,ament_uncrustify --reformat
will often make changes that causeament_clang_format
to complain and vice versa.One of these tools should be deprecated or they should enforce compatible code formatting styles.
Formatted with
ament_uncrustify --format
:formatted with
ament_clang_format --reformat
:The text was updated successfully, but these errors were encountered: