Skip to content

Conversation

@johnbowen42
Copy link
Contributor

@johnbowen42 johnbowen42 commented Oct 8, 2024

This is a refactoring PR that uses a script to insert annotations in cpp source code. It inserts annotations before and after templated function calls or declarations inside the examples and exercises subdirectories, as these tend to have the most complex examples of such calls and declarations.

see also #1731

@johnbowen42 johnbowen42 changed the title feature: script to toggle clang-format on/off feature: script to toggle clang-format on/off, clang-format exercises and examples Oct 14, 2024
@johnbowen42 johnbowen42 requested review from a team, artv3 and rhornung67 October 14, 2024 18:27
RAJA::ReduceMinLoc<reduce_policy, int> kernel_minloc(
std::numeric_limits<int>::max(), -1);
RAJA::ReduceMaxLoc<reduce_policy, int> kernel_maxloc(
std::numeric_limits<int>::min(), -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this an improvement, what do others think? @LLNL/raja-core ?

>
>
>;
using kernel_pol = RAJA::KernelPolicy<RAJA::statement::HipKernelFixed<
Copy link
Member

@rhornung67 rhornung67 Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes like this (and throughout) are not good. It's not consistent and hinders readability. The script should turn off clang-format for RAJA::kernel policies in all tests, examples, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just didn't apply the script to the benchmarks subdirectory

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should not be applied to exercises, examples, and possibly tests (I need to look at those) based on what I am seeing.

Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs work to filter out formatting of nested template code sections, especially involving RAJA::kernel.

@johnbowen42
Copy link
Contributor Author

This still needs work to filter out formatting of nested template code sections, especially involving RAJA::kernel.

could you clarify if there are any issues in the examples of exercises directories? I didn't apply the script to the benchmarks directory

using Pol = KernelPolicy<
Collapse<omp_target_parallel_collapse_exec, ArgList<0,1>, Lambda<0> > >;
// clang-format on
Collapse<omp_target_parallel_collapse_exec, ArgList<0, 1>, Lambda<0>>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of what I don't like. Putting all the closing '>' on the same line makes it harder to parse visually. I prefer that the indentation pattern that we follow consistently for the most part, where opening '<' and 'closing '>' are vertically aligned in execution policies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the closing > were all on the same line before clang format was applied, in this case


*atomic_pi = 0.0;

// clang-format off
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the script insert the clangt-format off comment here, but not at line 129 above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants