Create a clang-format config

[taketwo]

We have #2376 that started to deal with this topic, however the discussion there lost focus. I propose to use this issue to discuss exclusively the style config (no CI, clang-tidy, strategies to enforce, etc).

Here is a tentative config file that was born out of #2376:

---
AccessModifierOffset: '0'
AlwaysBreakAfterReturnType : All
AlwaysBreakTemplateDeclarations: 'true'
BreakBeforeBraces: Linux
Language: Cpp
NamespaceIndentation: All
SpaceBeforeParens: Always
Standard: Cpp11
TabWidth: 2
UseTab: Never

The biggest open question was the line width. Here is how the distribution of line lengths look like in cpp and h files in PCL right now (only lines over 10 columns are shown):

My proposal was 120, but Sergio was pushing towards a smaller number.

One other thought that I had is the "SpaceBeforeParens" rule. Do we want to keep it? We will be touching most lines because of namespace indentation anyway, so can as well drop this notorious space.

Any other ideas/concerns/opinions?

[SergioRAgostinho]

My proposal was 120, but Sergio was pushing towards a smaller number.

How about we do 88, like black in python. Python has more restriction with indentation (mandatory and 4 spaces) than C++ (we're using 2) and everyone enjoys black. It will cover that spike in the histogram.

One other thought that I had is the "SpaceBeforeParens" rule. Do we want to keep it? We will be touching most lines because of namespace indentation anyway, so can as well drop this notorious space.

I'm totally ok in dropping it. Have no real affection for it and it will always makes transitioning between other languages/projects (python, matlab, every other c++ project) and PCL a little less awkward than right now.

[kunaltyagi]

For me, 100 chars is an upper bound because otherwise it's not possible to open 2 editors side by side and compare the code with ease (peek at headers/something else) on a laptop. For comparison: my IDE can show a maximum of 125 chars after all of the real-estate is taken over by other things floating left and right.

Prefer any value which allows easy reading of code, looking at at least 2 files side-by-side and typical workflow requirements of the developers. The actual value doesn't matter much.

I usually switch off namespace indentation because almost everything is in a namespace. No need to add unneeded white-space.

Stuff I wish is in the style for a lot of code (since we are touching almost every line):

• no one-line-blocks after for, if, etc.: helps prevent silly mistakes, but I recently saw clang issue a warning about such code, so maybe this isn't needed. (sidenote) This does however reduce diffs on adding other stuff under the conditionals.

  if(statement)
  do_this();
  do_that();
  // vs
  if (statement)
  {
  do_this();
  }
  do_that();

[taketwo]

Python has more restriction with indentation (mandatory and 4 spaces) than C++ (we're using 2)

True, though on the other hand Python does not have horrible typename const class<template> combos to specify variable types. But OK, if you want 88, let it be 88, I'm not against. How does PCL code look when formatted to 88 chars?

"SpaceBeforeParens" rule. I'm totally ok in dropping it.

:+1:

I usually switch off namespace indentation because almost everything is in a namespace. No need to add unneeded white-space.

Same sentiment from my side.

no one-line-blocks after for, if, etc.:

I wouldn't be against. This brings another question though, positioning of {. I've been a proponent of { on it's own line for a long time, but switched to if (blah) { style a couple of years ago and never looked back. Saves tons of screen space.

[kunaltyagi]

but switched to if (blah) { style a couple of years ago

I made the switch to the same (new line after functions, and before else; none otherwise). Gives visibility to functions, saves screen space.

BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: true
  AfterClass: false
  AfterStruct: false
  BeforeElse: true
  SplitEmptyFunction: false
  SplitEmptyRecord: false
  SplitEmptyNamespace: false

For better visuals, I also use the lesser known properties

BinPackArguments: false.  # either on one line, or one line per argument (like Python)
BinPackParameters: false. # same as above, but for parameters
PointerAlignment: Right.  # int *ptr, value; instead of int* ptr, value;
SpacesInCStyleCastParentheses: true  # ( int )9.0 makes the casts stand out.

How does PCL code look when formatted to 88 chars

I made a push. Do check it out. It's a bit hard to see which lines were affected by column width. Maybe you have some sample files (based on the graph above)

[SergioRAgostinho]

I have a strong preference against

PointerAlignment: Right.  # int *ptr, value; instead of int* ptr, value;
SpacesInCStyleCastParentheses: true  # ( int )9.0 makes the casts stand out.

The first item especially. Everything else, I don't really care.

[kunaltyagi]

No issues. Personal config's are supposed to be very opinionated.

[taketwo]

I made the switch to the same (new line after functions, and before else; none otherwise).

Interesting combination, did not see it before.

PointerAlignment: Right.

I am also against, but not strong at all. My reason is that semantically asterisk is a part of type (int and int* are different types), as such it should be glued to the type name, not to the variable name.

SpacesInCStyleCastParentheses: true

Ideally just eliminate all c casts :)

[SergioRAgostinho]

I am also against, but not strong at all. My reason is that semantically asterisk is a part of type (int and int* are different types), as such it should be glued to the type name, not to the variable name.

Exactly.

I made a push. Do check it out. It's a bit hard to see which lines were affected by column width. Maybe you have some sample files (based on the graph above)

I remember the moving least squares code to be somewhat messy. https://github.com/PointCloudLibrary/pcl/blob/master/surface/include/pcl/surface/impl/mls.hpp https://github.com/kunaltyagi/pcl/blob/c88/surface/include/pcl/surface/impl/mls.hpp

Also the infamous agast if-else blocks https://github.com/kunaltyagi/pcl/blob/c88/keypoints/src/agast_2d.cpp https://github.com/PointCloudLibrary/pcl/blob/master/keypoints/src/agast_2d.cpp

[taketwo]

Looks tidy now :)

@kunaltyagi Was this formatted with all your proposed options? In particular, you brought up

BinPackArguments: false.  # either on one line, or one line per argument (like Python)

which I did not know about, so was curious to see in action. But the code does not seem to have one argument per line.

[kunaltyagi]

Was this formatted with all your proposed options?

No. This is the current .clang-format. I've not modified it from the PCL guidelines

[kunaltyagi]

so was curious to see in action.

I got some time so I pushed another branch: c88-binpack and a PR so that you can compare easily.

[taketwo]

Thanks. I think I like the readability of that binpack brings.

Regarding 88... well, sometimes even the function name (qualified) does not fit in one line :sweat_smile:

template <typename PointT, typename LeafContainerT, typename BranchContainerT>
void
pcl::octree::OctreePointCloudSearch<PointT, LeafContainerT, BranchContainerT>::
    approxNearestSearch (int query_index, int &result_index, float &sqr_distance)
{

Given your arguments, I guess my 120 proposal is waaaay off the radar for you. So I'm not going to fight about this one :smile:

[kunaltyagi]

I think I like the readability of that binpack brings.

Okay. Let's see if others like it too. 😄

sometimes even the function name (qualified) does not fit in one line

That's my issue with namespaces (to a degree) and templates.

We can consider

• using inline namespace (not sufficient mileage)
• using namespace in impl files (not sufficient mileage)
• renaming classes (can it simply be OctreeSearch instead?) and use the following

  template <typename A, typename B, typename C>
  using OctreePointCloudSearch[
  [deprecated ("Please consider using OctreeSearch instead")]] =
  OctreeSearch<A, B, C>;

Or we can do a refactor. For classes with too many templates, we can separate the config into a traits struct. Combined with inline namespace octree, we end up with the following code (except inline, this breaks the API sharply). The following code is not something we'd attain easily, so it's just an example.

namespace pcl
{
  inline namespace octree
  {
    template <typename LeafC = OctreeContainerPointIndices,
              typename BranchC = OctreeContainerEmpty>
    struct DefaultSearchConfig {
      using LeafContainerT = LeafC;
      using BranchContainerT = BranchC;
    };
    template <typename T>
    constexpr auto is_octree_search_config_v = some_filler_code<T>::value;
    // ^^^^^^^^ enforced by static_assert in class body
  } // namespace octree
} // namespace pcl
namespace pcl
{
  template <typename PointT, typename Config = DefaultSearchConfig<>>
  void
  pcl::OctreePointCloudSearch<PointT, Config>::approxNearestSearch (int query_index,
                                                                    int &result_index,
                                                                    float &sqr_distance)
  { // actual function impl
  }
} // namespace pcl

For compatibility, there can be a shim class which is deprecated. Lot's of work, I don't recommend for anything except parts of the library which are unstable and need an emergency upgrade. Octree looks very stable and I'd stay away from this option.

[SergioRAgostinho]

I think I like the readability of that binpack brings.

Okay. Let's see if others like it too. 😄

I'm ok with it. :+1:

Everything else, it's a level of beautification I don't personally consider important.

[taketwo]

I'd like to propose a few more options controlling constructor initializer lists. PCL is not 100% consistent on this, but I think our style is to break the list one variable per line and put comma before:

  FooClass()
  : vpx_(0)
  , vpy_(0)
  , vpz_(0)
  , leaf_size_(0.005f)
  , normalize_bins_(false)
  , curv_threshold_(0.03f)
  , cluster_tolerance_(leaf_size_ * 3)
  , eps_angle_threshold_(0.125f)
  , min_points_(50)
  , radius_normals_(leaf_size_ * 3)
  {
    // implementation

It's more readable (in a similar way to argument bin packing), it's easier to reorder, and yields smaller diffs on changes. Here are corresponding options:

    ConstructorInitializerAllOnOneLineOrOnePerLine : true
    BreakConstructorInitializers : BeforeComma
    ConstructorInitializerIndentWidth : 0

I'm open for different indentations, though personally find it nice when commas are aligned with {.

[SergioRAgostinho]

I agree with the proposal and don't have any preference regarding indentation.

[taketwo]

---
AlwaysBreakAfterReturnType: All
AlwaysBreakTemplateDeclarations: 'true'
BinPackArguments: 'false'
BinPackParameters: 'false'
BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: true
  AfterClass: false
  AfterStruct: false
  BeforeElse: true
  SplitEmptyFunction: false
  SplitEmptyRecord: false
  SplitEmptyNamespace: false
BreakConstructorInitializers: BeforeComma
ColumnLimit: '88'
ConstructorInitializerAllOnOneLineOrOnePerLine: 'true'
ConstructorInitializerIndentWidth: '0'
Language: Cpp
PointerAlignment: Left
Standard: Cpp11
TabWidth: '2'
UseTab: Never

Did I miss anything?

[kunaltyagi]

Not really.

Take a look at PR for comparison and the code at branch code-style-draft

One last (non-)question from my side though. Should I add the .clang-format as a separate commit (to resolve this issue) or included with the format script and relevant cmake changes (one PR resolving multiple issues)?

[taketwo]

Thanks. I skimmed through (some of) the files and this looks good to me. I noticed an issue in "2d/convolution.h":

We'll need to prevent clang-format from breaking our formatting. This can be achieved by adding // at the end of each line. This has nothing to do with the config though, but we should remember since there will be many cases in "common" when we want to preserve custom formatting.

I don't think we necessarily need a separate PR for the config. I'd add it as a separate commit to #3188.

[kunaltyagi]

We'll need to prevent clang-format from breaking our formatting. This can be achieved by adding // at the end of each line.

Wouldn't a better method would be // clang-format on and // clang-format off functionality provided in clang-format

I'd add it as a separate commit to #3188.

Sending an updated commit soon

[taketwo]

The directive approach is also possible, though I find // to look more neat than adding extra lines around. But that's a matter of taste, of course.

POINT_CLOUD_REGISTER_POINT_STRUCT(pcl::InterestPoint,         //
                                  (float, x, x)               //
                                  (float, y, y)               //
                                  (float, z, z)               //
                                  (float, strength, strength) //
)

There is a practical plus to the // approach: the code still goes through formatting, so other style aspects (besides from line breaks) are formatted to our style.