ClangFormat Style

[crtrott]

@dalg24 and @Rombur requested that we add a clang-format style. The following is what I like and what is as far as I can tell the closest to the style we have been employing manually:

BasedOnStyle: google
AlignConsecutiveAssignments: true
AllowShortCaseLabelsOnASingleLine: true
AllowShortIfStatementsOnASingleLine: true
#AllowShortLambdasOnASingleLine: true

The AllowShortLambdasOnASingleLine is a pretty recent clang only, so I commented that out for now. This is an example test code:

#define A_MACRO \
  int a;     \
int bbbbb  ;\
double dddddddddddddd; \
int e;

namespace ANameSpace
{
namespace BNameSpace {

template<class FoolMe, class B,
 class C>
struct Foo {
  Foo() {};
  int a;
  double b,c;
  const double e=5.0;
  const int an_int = 5;

  void foo() {}
  int bar_me() { return 0; }
  double tree (int a, int b,
    int call_me_later = 0,
  double me_too1 = 1.0,
  double me_too2 = 1.0,
  double me_too3 = 1.0,
  double me_too4 = 1.0,
  double me_too5 = 1.0,
  double me_too = 1.0) {
    return 1.0;
  }
  double tree_with_very_very_very_very_long_function_name (int a, int b,
    int call_me_later = 0,
  double me_too1 = 1.0,
  double me_too2 = 1.0,
  double me_too3 = 1.0,
  double me_too4 = 1.0,
  double me_too5 = 1.0,
  double me_too = 1.0) {
    switch (call_me_later) {
      case 1:
        return 1.0;
        break;
      case 2: return 2.0; break;
      case 3:
        me_too *= me_too1;
        me_too *= me_too2;
        me_too *= me_too3;
        me_too *= me_too4;
      break;}
  }
  if(me_too < 0)
    return 0;
  else {
    me_too += me_too1;
    me_too += me_too2;
  return me_too;
  }

  void
  break_after_return_type(int a) {
    if(a==0) {
      printf("Huch\n");}
else {
      printf("OhNo\n");
    }
  }
};

void mean (int a,
  int b = 0, double gamma = 0.0,
 int64_t delta = 0 ) {
  b = 1;
  gamma = 5;
  delta = 1; b = 2;


  Foo<int, double,double    > f;
  f.foo();
  f.tree(a,  b,delta);


  b = 5 + // First thing
   3;// Second Thing
}

}}

void args (int a)
{
  printf("Too\n");
}

void args (int a)
{
  printf("Too\n");
  printf("Too1\n");
  printf("Too2\n");
  printf("Too3\n");
  printf("Too4\n");
}

int for_loop (int a) {
  int sum= 0;
  for(int i=0; i<a; i++) {
    sum+=i*a;
  }
  for(int i=0; i<a; i++) sum+=1;

  return sum;
}

Formatted:

#define A_MACRO          \
  int a;                 \
  int bbbbb;             \
  double dddddddddddddd; \
  int e;

namespace ANameSpace {
namespace BNameSpace {

template <class FoolMe, class B, class C>
struct Foo {
  Foo(){};
  int a;
  double b, c;
  const double e   = 5.0;
  const int an_int = 5;

  void foo() {}
  int bar_me() { return 0; }
  double tree(int a, int b, int call_me_later = 0, double me_too1 = 1.0,
              double me_too2 = 1.0, double me_too3 = 1.0, double me_too4 = 1.0,
              double me_too5 = 1.0, double me_too = 1.0) {
    return 1.0;
  }
  double tree_with_very_very_very_very_long_function_name(
      int a, int b, int call_me_later = 0, double me_too1 = 1.0,
      double me_too2 = 1.0, double me_too3 = 1.0, double me_too4 = 1.0,
      double me_too5 = 1.0, double me_too = 1.0) {
    switch (call_me_later) {
      case 1: return 1.0; break;
      case 2: return 2.0; break;
      case 3:
        me_too *= me_too1;
        me_too *= me_too2;
        me_too *= me_too3;
        me_too *= me_too4;
        break;
    }
  }
  if (me_too < 0)
    return 0;
  else {
    me_too += me_too1;
    me_too += me_too2;
    return me_too;
  }

  void break_after_return_type(int a) {
    if (a == 0) {
      printf("Huch\n");
    } else {
      printf("OhNo\n");
    }
  }
};

void mean(int a, int b = 0, double gamma = 0.0, int64_t delta = 0) {
  b     = 1;
  gamma = 5;
  delta = 1;
  b     = 2;

  Foo<int, double, double> f;
  f.foo();
  f.tree(a, b, delta);

  b = 5 +  // First thing
      3;   // Second Thing
}

}  // namespace BNameSpace
}  // namespace ANameSpace

void args(int a) { printf("Too\n"); }

void args(int a) {
  printf("Too\n");
  printf("Too1\n");
  printf("Too2\n");
  printf("Too3\n");
  printf("Too4\n");
}

int for_loop(int a) {
  int sum = 0;
  for (int i = 0; i < a; i++) {
    sum += i * a;
  }
  for (int i = 0; i < a; i++) sum += 1;

  return sum;
}

If anyone has any gripes about this let me know. The plan would be to apply this after the 2.9 release. The 3.0 release is not supposed to have any code changes, but just the build system overhaul, so it is a good target to do a wholesale reformatting.

[dhollman]

I really dislike AlignConsecutiveAssignments, but I don’t have a good reason for it. Everything else is a major improvement.

[crtrott]

I know David, but I like it and as long as not a healthy majority of the team agrees with you I win :-P

[dhollman]

Yeah, of course. Like I said, I can’t come up with a particularly good reason why I don’t like it. I was just logging a data point.

[dalg24]

--- old	2019-05-28 17:25:36.000000000 -0400
+++ new	2019-05-28 17:25:53.000000000 -0400
@@ -1,98 +1,79 @@
-#define A_MACRO \
-  int a;     \
-int bbbbb  ;\
-double dddddddddddddd; \
-int e;
+#define A_MACRO          \
+  int a;                 \
+  int bbbbb;             \
+  double dddddddddddddd; \
+  int e;

-namespace ANameSpace
-{
+namespace ANameSpace {
 namespace BNameSpace {

-template<class FoolMe, class B,
- class C>
+template <class FoolMe, class B, class C>
 struct Foo {
-  Foo() {};
+  Foo(){};
   int a;
-  double b,c;
-  const double e=5.0;
+  double b, c;
+  const double e   = 5.0;
   const int an_int = 5;

   void foo() {}
   int bar_me() { return 0; }
-  double tree (int a, int b,
-    int call_me_later = 0,
-  double me_too1 = 1.0,
-  double me_too2 = 1.0,
-  double me_too3 = 1.0,
-  double me_too4 = 1.0,
-  double me_too5 = 1.0,
-  double me_too = 1.0) {
+  double tree(int a, int b, int call_me_later = 0, double me_too1 = 1.0,
+              double me_too2 = 1.0, double me_too3 = 1.0, double me_too4 = 1.0,
+              double me_too5 = 1.0, double me_too = 1.0) {
     return 1.0;
   }
-  double tree_with_very_very_very_very_long_function_name (int a, int b,
-    int call_me_later = 0,
-  double me_too1 = 1.0,
-  double me_too2 = 1.0,
-  double me_too3 = 1.0,
-  double me_too4 = 1.0,
-  double me_too5 = 1.0,
-  double me_too = 1.0) {
+  double tree_with_very_very_very_very_long_function_name(
+      int a, int b, int call_me_later = 0, double me_too1 = 1.0,
+      double me_too2 = 1.0, double me_too3 = 1.0, double me_too4 = 1.0,
+      double me_too5 = 1.0, double me_too = 1.0) {
     switch (call_me_later) {
-      case 1:
-        return 1.0;
-        break;
+      case 1: return 1.0; break;
       case 2: return 2.0; break;
       case 3:
         me_too *= me_too1;
         me_too *= me_too2;
         me_too *= me_too3;
         me_too *= me_too4;
-      break;}
+        break;
+    }
   }
-  if(me_too < 0)
+  if (me_too < 0)
     return 0;
   else {
     me_too += me_too1;
     me_too += me_too2;
-  return me_too;
+    return me_too;
   }

-  void
-  break_after_return_type(int a) {
-    if(a==0) {
-      printf("Huch\n");}
-else {
+  void break_after_return_type(int a) {
+    if (a == 0) {
+      printf("Huch\n");
+    } else {
       printf("OhNo\n");
     }
   }
 };

-void mean (int a,
-  int b = 0, double gamma = 0.0,
- int64_t delta = 0 ) {
-  b = 1;
+void mean(int a, int b = 0, double gamma = 0.0, int64_t delta = 0) {
+  b     = 1;
   gamma = 5;
-  delta = 1; b = 2;
-
+  delta = 1;
+  b     = 2;

-  Foo<int, double,double    > f;
+  Foo<int, double, double> f;
   f.foo();
-  f.tree(a,  b,delta);
+  f.tree(a, b, delta);

-
-  b = 5 + // First thing
-   3;// Second Thing
+  b = 5 +  // First thing
+      3;   // Second Thing
 }

-}}
+}  // namespace BNameSpace
+}  // namespace ANameSpace

-void args (int a)
-{
-  printf("Too\n");
-}
+void args(int a) { printf("Too\n"); }

-void args (int a)
-{
+void args(int a) {
   printf("Too\n");
   printf("Too1\n");
   printf("Too2\n");
@@ -100,12 +81,12 @@
   printf("Too4\n");
 }

-int for_loop (int a) {
-  int sum= 0;
-  for(int i=0; i<a; i++) {
-    sum+=i*a;
+int for_loop(int a) {
+  int sum = 0;
+  for (int i = 0; i < a; i++) {
+    sum += i * a;
   }
-  for(int i=0; i<a; i++) sum+=1;
+  for (int i = 0; i < a; i++) sum += 1;

   return sum;
 }

[dhollman]

For the sake (unnecessary) thoroughness, my main reason for disliking AlignConsecutiveAssignments is that I surprisingly often write things like:

int i                                                                                   = 0;
double val                                                                              = 0.0;
Kokkos::View<double**, typename ExecutionSpace::memory_space, Kokkos::LayoutRight> data = { };

which is pretty unreadable. If we can get better about always using auto on the left-hand side, this problem goes away.

The other problem is that, even with auto, this formatting can discourage people from using descriptive enough variable names. For instance:

auto value                                            = /*...*/;
auto factor                                           = /*...*/;
auto some_complicated_variable_that_needs_explanation = /*...*/;

The spacing makes it feel like the person with the descriptive variable name is doing something wrong, when in fact it may be necessary to increase the readability of the program.

Again, though, I really don't care that much. I just wanted to log a data point. Any use of a formatting standard is so much better than what we have that I'm all for it.

(Also, we should make sure that automatic formatting is escapable. There is quite a bit of research to show that there are times when developers need to use spacing outside of a prescribed format to convey something to the reader of the code. I believe clang-format provides the hooks for doing this; it's just something to be aware of when deciding on the "strictness" of enforcement.)

[Rombur]

Also, we should make sure that automatic formatting is escapable

This can done using // clang-format off to disable clang-reformat and it can be re-enabled using // clang-format on

[crtrott]

Also it looks like there are ways how you can apply clang-format only to the changed lines. People have done scripts (i.e. github hooks) for that.

[crtrott]

I just ran the thing over the whole code base: 111k lines added, 106k lines removed out of 264k lines. This is what I did:

find * -iname *.hpp -o -iname *.cpp | xargs cat | wc
find * -iname *.hpp -o -iname *.cpp | xargs clang-format -i -style=file
git diff &> out
cat out | awk '{if (substr($1,1,1) ~ /^[+]/) print $1 }' |wc
cat out | awk '{if (substr($1,1,1) ~ /^[-]/) print $1 }' |wc

Based on a sample of the diff the changes are mostly very minor:

-  offset_type get_bin_offsets() const { return bin_offsets;}
+  offset_type get_bin_offsets() const { return bin_offsets; }

-  void operator() (const bin_binning_tag& tag, const int& i)  const {
-    const int j     = range_begin + i ;
-    const int bin   = bin_op.bin(keys,j);
+  void operator()(const bin_binning_tag& tag, const int& i) const {
+    const int j     = range_begin + i;
+    const int bin   = bin_op.bin(keys, j);

A lot of it is indentation for namespace: i.e. we often have code indented 2 spaces for almost everything. I.e. inside a namespace Kokkos { but we also indent only 2 spaces for tightly nested namespaces.

[dhollman]

This is going to be a nightmare for any outstanding branches; basically all changes in those branches will just be lost, I think. We need to be absolutely sure we want this and don’t want things on outstanding on branches before we make this change in develop

[crtrott]

New files will be ok but otherwise conflicts will be problematic. We will discuss this in todays meeting.

[masterleinad]

You just need someone volunteering to update all the pull requests. As long as you are continuously fixing formatting from newest to oldest commits, you should be fine.

[prckent]

You will need to specify the exact clang-format version(s) that are supported.

[crtrott]

No reordering of headers: needs to be explicitly turned off.

[crtrott]

This is now merged including the automated testing of compliance for pull requests.