rfbird commented 3 years ago

Currently we don't prescribe how pointers should be handled in the clang format file, and instead leave it to be inferred (via DerivePointerAlignment). This means:

We have inconsistent formatting in the code; and It allows different versions of clang format to do different things (which is currently causing me issues, where my format disagrees with Travis).

This sets pointer alignment for pointers and references to be "middle"

Fixes #297

rfbird commented 3 years ago

Travis is failing because the two clang formats disagree about how to handle function call params with nested function calls. i.e.:

-    grid_parallel_for(
-        "fill_rank_1", TEST_EXECSPACE(), is1,
-        KOKKOS_LAMBDA( const int i ) { v1( i ) = 1.0; } );

vs

+    grid_parallel_for( "fill_rank_1", TEST_EXECSPACE(), is1,
+                       KOKKOS_LAMBDA( const int i ) { v1( i ) = 1.0; } );

Locally I can't get either version of clang-format (9 or 11) to agree with Travis, and can't find an option for handling next function calls (only some old docs saying it's not supported).

Any ideas appreciated

dalg24 commented 3 years ago

Did you make sure you were using the exact same version of clang-format?

rfbird commented 3 years ago

Did you make sure you were using the exact same version of clang-format?

Nope, thats next. In general I'd prefer we prescribe the behavior (where possible) in the format file rather than relying on the user matching versions implicitly, etc

dalg24 commented 3 years ago

TO be more defensive you could also dump the entire configuration instead of relying implicitly on defaults like we do now.

rfbird commented 3 years ago

TO be more defensive you could also dump the entire configuration instead of relying implicitly on defaults like we do now.

That's a nice idea. Is there an elegant way to dump the config for travis, or shall I just hack it into the .travis file? (i.e add a needless call to clang-format -style=file -dump-config

rfbird commented 3 years ago

I fed the dumped format of clang-format v7 (which gives me results matching travis) into clang-format v11 and it gives me a different answer to v7, so I think something internal to clang-format changed..

codecov[bot] commented 3 years ago

Codecov Report

Merging #298 into master will not change coverage. The diff coverage is 87.1%.

@@          Coverage Diff           @@
##           master    #298   +/-   ##
======================================
  Coverage    93.4%   93.4%           
======================================
  Files          50      50           
  Lines        2944    2944           
======================================
  Hits         2750    2750           
  Misses        194     194

Flag	Coverage Δ
#clang	`93.4% <87.1%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cajita/src/Cajita_Array.hpp	`100.0% <ø> (ø)`
cajita/src/Cajita_GlobalGrid.cpp	`93.4% <ø> (ø)`
cajita/src/Cajita_GlobalGrid.hpp	`100.0% <ø> (ø)`
cajita/src/Cajita_GlobalMesh.hpp	`0.0% <0.0%> (ø)`
cajita/src/Cajita_IndexSpace.hpp	`0.0% <0.0%> (ø)`
cajita/src/Cajita_Interpolation.hpp	`100.0% <ø> (ø)`
cajita/src/Cajita_LocalGrid.cpp	`94.7% <ø> (ø)`
cajita/src/Cajita_LocalGrid.hpp	`100.0% <ø> (ø)`
cajita/src/Cajita_ManualPartitioner.hpp	`100.0% <ø> (ø)`
cajita/src/Cajita_Parallel.hpp	`100.0% <ø> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a8104e2...9b0be82. Read the comment docs.

junghans commented 3 years ago

format this please

rfbird commented 3 years ago

format this please

What happened here? 8 and 7 disagree? fun

junghans commented 3 years ago

format this please

What happened here? 8 and 7 disagree? fun

We should maybe port the CI from travis to GitHub Actions.

ECP-copa / Cabana

Change clang format to specify pointer alignment middle #298

Codecov Report