feature: Add make style target and refactor RAJA with clang-format #1731
Conversation
examples/kernel-dynamic-tile.cpp
Outdated
| > | ||
| > | ||
| >(make_tuple(RangeSegment{0,25}, RangeSegment{0,25}), | ||
| kernel_param<KernelPolicy<statement::Tile< |
There was a problem hiding this comment.
This is an example of making nested templates hard to read. I think the original format is much easier to read. @artv3 do you agree?
johnbowen42
force-pushed
the
feature/bowen/add-clang-format
branch
from
d7db3ce to
4d92935
Compare
|
@johnbowen42 About this PR...As we merge other PRs, we need to merge develop into this PR branch. So please post instructions for rerunning the formatting (which clang version, etc.) in the top description of this PR. Thank you. |
|
Also, @johnbowen42 I noticed that some header file inclusion orders were changed in this PR. I haven't dug in, but that may be the cause of the CI failures. |
johnbowen42
force-pushed
the
feature/bowen/add-clang-format
branch
from
25aee03 to
696caf4
Compare
Should be fixed: Line 9 in 696caf4 |
|
@LLNL/raja-core please drop this PR a review |
| RAJA_INLINE bool compareSegmentById( | ||
| size_t segid, | ||
| const TypedIndexSet<P0, PREST...> &other) const | ||
| RAJA_INLINE bool |
There was a problem hiding this comment.
Why does it put a line break here between the return type and method name, but not below at line 189?
| } | ||
| ); | ||
| forall<ExecPolicy<seq_segit, seq_exec>>( | ||
| iset, [&](typename CONTAINER_T::value_type idx) { tcon.push_back(idx); }); |
There was a problem hiding this comment.
I think it's better to start lambda expressions on a new line. Also, why did it not format the curly braces on new lines like other code sections?
There was a problem hiding this comment.
I have it set to allow single line lambdas
| tcon.push_back(idx); | ||
| } | ||
| ); | ||
| forall<seq_exec>(seg, [&](typename CONTAINER_T::value_type idx) |
There was a problem hiding this comment.
The formatting choices, like this one, are inconsistent with others.
| } | ||
| ); | ||
| forall<ExecPolicy<seq_segit, seq_exec>>( | ||
| iset, |
There was a problem hiding this comment.
why is it indenting 4 spaces here and 2 spaces in other places?
There was a problem hiding this comment.
it's applying an argument alignment rule instead of an indentation rule
| ); | ||
| forall<ExecPolicy<seq_segit, seq_exec>>( | ||
| iset, | ||
| [&](typename CONTAINER_T::value_type idx) |
There was a problem hiding this comment.
This is what I would expect in my comments above. It appears that it is formatting lambda expressions in multiple inconsistent ways.....grrrfff.
| struct StripIndexTypeT { | ||
| using type = FROM; | ||
| namespace internal | ||
| { |
There was a problem hiding this comment.
It would be nice if it inserted a blank line between namespace declarations and the contents inside.
| }; | ||
| } // namespace internal | ||
| } // namespace internal |
There was a problem hiding this comment.
Adding to an earlier comment, adding a blank line before the closing curly brace of a namespace would be preferred.
| struct StripIndexTypeT<FROM, typename std::enable_if<std::is_base_of<IndexValueBase, FROM>::value>::type> | ||
| template <typename FROM> | ||
| struct StripIndexTypeT< | ||
| FROM, |
There was a problem hiding this comment.
This template parameter formatting is strange.
| using IndexValueType = TYPE; \ | ||
| RAJA_HOST_DEVICE RAJA_INLINE TYPE() : parent::IndexValue() {} \ | ||
| RAJA_HOST_DEVICE RAJA_INLINE explicit TYPE(::RAJA::Index_type v) \ | ||
| : parent::IndexValue(v) \ |
| public: \ | ||
| RAJA_HOST_DEVICE RAJA_INLINE TYPE() \ | ||
| : RAJA::IndexValue<TYPE, IDXT>::IndexValue() \ | ||
| {} \ |
There was a problem hiding this comment.
Is there a way to make it add an empty line between methods?
|
please see this pull request for a simplified version #1767 |
This PR refactors the entire RAJA codebase using a new
make stylecode target. Thestyletarget is configured in cmake using theraja_add_code_checksmacro, which uses much of the same logic asaxom_add_code_checksmacro in axom.To apply the formatting specified by this PR, simply call
make stylefrom the build directory, and specify a validclang-format14 version viaCLANGFORMAT_EXECUTABLEupdate 10/1: I removed examples and exercises from this PR, with the intention of reformatting them once I create a script for excluding compound template types from clang formatting. The plan is to
(1) merge this PR once everyone is satisfied with the formatting rules
(2) Add a pipeline that formats and enforces style
(3) Add a script that excludes nested typedefs automatically in a follow-up