feature: Add make style target and refactor RAJA with clang-format #1767

johnbowen42 · 2024-11-13T21:09:27Z

This PR applies clang-format to the include and src directories of develop

johnbowen42 · 2024-11-13T21:40:51Z

@rhornung67 and I worked on the settings this morning. Here is what we settled on:

Lines 12 to 16 in c5bee85

    
           # Alignment of consecutive declarations, assignments etc 
        
           AlignConsecutiveAssignments : true 
        
           AlignConsecutiveDeclarations : false 
        
           AlignConsecutiveMacros : true 
        
           AlignTrailingComments : true

example:

RAJA/include/RAJA/index/IndexValue.hpp

Lines 53 to 57 in c5bee85

    
           RAJA_INLINE constexpr IndexValue()                   = default; 
        
           constexpr RAJA_INLINE IndexValue(IndexValue const&)  = default; 
        
           constexpr RAJA_INLINE IndexValue(IndexValue&&)       = default; 
        
           RAJA_INLINE IndexValue& operator=(IndexValue const&) = default; 
        
           RAJA_INLINE IndexValue& operator=(IndexValue&&)      = default;

RAJA/.clang-format

Lines 20 to 37 in c5bee85

    
           BraceWrapping: 
        
             AfterCaseLabel:  true 
        
             AfterClass:      true 
        
             AfterControlStatement: true 
        
             AfterEnum:       true 
        
             AfterFunction:   true 
        
             AfterNamespace:  true 
        
             AfterObjCDeclaration: false 
        
             AfterStruct:     true 
        
             AfterUnion:      true 
        
             AfterExternBlock: false 
        
             BeforeCatch:     true 
        
             BeforeElse:      true 
        
             BeforeLambdaBody: false 
        
             IndentBraces:    false 
        
             SplitEmptyFunction: false 
        
             SplitEmptyRecord: false 
        
             SplitEmptyNamespace: false

This transparently controls whether a curly brace will occur on the next line following anything requiring a curly brace. Basically the only time we don't enforce curly brace on next line is for lambda body definitions and empty function definitions. Another notable exception is single line lambdas:

RAJA/.clang-format

Line 47 in c5bee85

AllowShortLambdasOnASingleLine : None

RAJA/.clang-format

Line 50 in c5bee85

BinPackArguments : true

a function call’s arguments will either be all on the same line or will have one line each.

RAJA/include/RAJA/index/IndexSetUtils.hpp

Lines 51 to 54 in c5bee85

    
           CONTAINER_T tcon; 
        
           forall<ExecPolicy<seq_segit, seq_exec>>( 
        
               iset, [&](typename CONTAINER_T::value_type idx) { 
        
                 tcon.push_back(idx);

RAJA/include/RAJA/index/IndexSetUtils.hpp

Lines 68 to 72 in c5bee85

    
           { 
        
             CONTAINER_T tcon; 
        
             forall<seq_exec>(seg, [&](typename CONTAINER_T::value_type idx) { 
        
               tcon.push_back(idx); 
        
             });

The difference here is caused by

RAJA/.clang-format

Line 53 in c5bee85

ColumnLimit : 80

RAJA/.clang-format

Line 51 in c5bee85

BinPackParameters : false
- This doesn't enforce the same packing for parameters in function declarations

RAJA/.clang-format

Line 56 in c5bee85

AlignAfterOpenBracket: Align

example

RAJA/include/RAJA/pattern/forall.hpp

Lines 148 to 155 in c5bee85

    
             RAJA_INLINE camp::resources::EventProxy<Res> operator()(T const&, 
        
                                                                     ExecPol, 
        
                                                                     Body, 
        
                                                                     Res, 
        
                                                                     ForallParams) const; 
        
             const int start; 
        
           };

RAJA/.clang-format

Line 67 in c5bee85

LambdaBodyIndentation : Signature

disallow single line lambdas, and indent the body one indent level relative to the indent level of the signature, e.g.

RAJA/include/RAJA/index/IndexSetUtils.hpp

Lines 109 to 114 in c5bee85

    
           { 
        
             CONTAINER_T tcon; 
        
             forall<seq_exec>(seg, [&](typename CONTAINER_T::value_type idx) { 
        
               if (conditional(idx)) tcon.push_back(idx); 
        
             }); 
        
             con = tcon;

RAJA/.clang-format

Line 69 in c5bee85

SeparateDefinitionBlocks : Always

enforce spacing between functions and other blocks, e.g.

RAJA/include/RAJA/index/IndexValue.hpp

Lines 250 to 259 in c5bee85

    
           { 
        
             return (value >= x.value); 
        
           } 
        
           RAJA_HOST_DEVICE RAJA_INLINE bool operator==(value_type x) const 
        
           { 
        
             return (value == x); 
        
           } 
        
           RAJA_HOST_DEVICE RAJA_INLINE bool operator==(TYPE x) const

MrBurmark · 2024-12-05T16:22:33Z

include/RAJA/index/IndexSetUtils.hpp

-    }
-  );
+  forall<seq_exec>(seg, [&](typename CONTAINER_T::value_type idx) {
+    if (conditional(idx)) tcon.push_back(idx);


We're allowing no-curly braces in single line conditionals?

I think we may have to move to clang-format 15 according to this: llvm/llvm-project@77e60bc

Note that we also get things like this currently: https://github.com/LLNL/RAJA/pull/1767/files#diff-e97c4ae7afcd5abc30afbee9cca754dcab6a4b2415e6396be367fd2d93103e22R54

MrBurmark · 2024-12-05T16:31:02Z

include/RAJA/pattern/WorkGroup/Dispatcher.hpp

  static constexpr bool use_host_invoke = dispatcher_use_host_invoke(platform);
-  using dispatch_policy = ::RAJA::direct_dispatch<T>;
-  using void_ptr_wrapper = DispatcherVoidPtrWrapper<DispatcherID>;
+  using dispatch_policy                 = ::RAJA::direct_dispatch<T>;
+  using void_ptr_wrapper  = DispatcherVoidPtrWrapper<DispatcherID>;
  using void_cptr_wrapper = DispatcherVoidConstPtrWrapper<DispatcherID>;


Interesting indentation matching choice.

MrBurmark · 2024-12-05T16:35:38Z

include/RAJA/pattern/WorkGroup/WorkStorage.hpp

+      : m_vec(0, aloc),
+        m_aloc(aloc)


I guess my way of placing commas was not normal. Hopefully clang format understands that it can't pull commas out of ifdefs.

MrBurmark · 2024-12-05T16:37:23Z

include/RAJA/pattern/WorkGroup/WorkStorage.hpp

+    if (loop_storage_size > storage_capacity())
+    {


Looks like we decided to put the curly on the line after conditionals.

Yes. John discussed this in a recent meeting.

MrBurmark · 2024-12-05T16:40:49Z

include/RAJA/pattern/detail/multi_reduce.hpp

+    using Base   = reduce::detail::BaseMultiReduce##OP_NAME<                   \
+        DATA<T, RAJA::reduce::OP<T>, tuning>>;                               \
+    using Base::Base;                                                          \


I guess something confused its indentation alignment.

I think clang-format has problems with making nested templates look pretty and readable, in general. That's why we won't be running it in the examples, exercises, and test directories.

johnbowen42 · 2024-12-13T01:27:38Z

docs/sphinx/dev_guide/contributing.rst

+
+
+
+#. The CMake build target `style`


@rhornung67 I added docs to this PR if you'd like to review

docs/sphinx/dev_guide/contributing.rst

rhornung67

I'm good with most of the formatting choices. There is still some weirdness involving nested templates, and it's not entirely clear to me when opening curly braces should appear on a new line or at the end of the current line. Hopefully, we can make adjustments as we get used to this.

johnbowen42 mentioned this pull request Nov 13, 2024

feature: Add make style target and refactor RAJA with clang-format #1731

Closed

johnbowen42 requested review from MrBurmark, artv3, a team, adayton1, rhornung67 and rchen20 November 13, 2024 21:52

johnbowen42 force-pushed the feature/bowen/add-clang-format-v2 branch from c11bf7c to 9a33df6 Compare November 14, 2024 23:33

artv3 approved these changes Nov 21, 2024

View reviewed changes

MrBurmark reviewed Dec 5, 2024

View reviewed changes

johnbowen42 added 3 commits December 12, 2024 11:32

Add CMake style target and githooks for formatting

f279264

Merge branch 'develop' into feature/bowen/add-clang-format-v2

11cc30b

Apply make style

11ccdbc

johnbowen42 force-pushed the feature/bowen/add-clang-format-v2 branch from 3a680ce to 11ccdbc Compare December 12, 2024 19:39

johnbowen42 and others added 2 commits December 12, 2024 15:25

add docs about style

65a74cb

Merge branch 'develop' into feature/bowen/add-clang-format-v2

afc557d

johnbowen42 commented Dec 13, 2024

View reviewed changes

johnbowen42 added 2 commits December 12, 2024 18:05

tweak docs

81f66a4

Update docs

ffc4f56

artv3 reviewed Dec 16, 2024

View reviewed changes

docs/sphinx/dev_guide/contributing.rst Outdated Show resolved Hide resolved

rhornung67 reviewed Dec 16, 2024

View reviewed changes

docs/sphinx/dev_guide/contributing.rst Outdated Show resolved Hide resolved

rhornung67 reviewed Dec 16, 2024

View reviewed changes

docs/sphinx/dev_guide/contributing.rst Outdated Show resolved Hide resolved

rhornung67 reviewed Dec 16, 2024

View reviewed changes

docs/sphinx/dev_guide/contributing.rst Outdated Show resolved Hide resolved

Docs

77a5738

rhornung67 approved these changes Dec 19, 2024

View reviewed changes

Merge branch 'develop' into feature/bowen/add-clang-format-v2

e0268ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add make style target and refactor RAJA with clang-format #1767

feature: Add make style target and refactor RAJA with clang-format #1767

johnbowen42 commented Nov 13, 2024

johnbowen42 commented Nov 13, 2024 •

edited

Loading

MrBurmark Dec 5, 2024

rhornung67 Dec 18, 2024 •

edited

Loading

MrBurmark Dec 5, 2024

MrBurmark Dec 5, 2024

MrBurmark Dec 5, 2024

rhornung67 Dec 19, 2024

MrBurmark Dec 5, 2024

rhornung67 Dec 19, 2024

johnbowen42 Dec 13, 2024 •

edited

Loading

rhornung67 left a comment

feature: Add make style target and refactor RAJA with clang-format #1767

Are you sure you want to change the base?

feature: Add make style target and refactor RAJA with clang-format #1767

Conversation

johnbowen42 commented Nov 13, 2024

johnbowen42 commented Nov 13, 2024 • edited Loading

MrBurmark Dec 5, 2024

Choose a reason for hiding this comment

rhornung67 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

MrBurmark Dec 5, 2024

Choose a reason for hiding this comment

MrBurmark Dec 5, 2024

Choose a reason for hiding this comment

MrBurmark Dec 5, 2024

Choose a reason for hiding this comment

rhornung67 Dec 19, 2024

Choose a reason for hiding this comment

MrBurmark Dec 5, 2024

Choose a reason for hiding this comment

rhornung67 Dec 19, 2024

Choose a reason for hiding this comment

johnbowen42 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

rhornung67 left a comment

Choose a reason for hiding this comment

johnbowen42 commented Nov 13, 2024 •

edited

Loading

rhornung67 Dec 18, 2024 •

edited

Loading

johnbowen42 Dec 13, 2024 •

edited

Loading