EAMxx: Formats all MAM4xx interfaces using Clang #7036
Conversation
singhbalwinder
requested review from
odiazib,
bartgol,
overfelt,
kaizhangpnl,
mjs271,
jaelynlitz and
mahf708
singhbalwinder
added
EAMxx
There was a problem hiding this comment.
wanna add an automated workflow check to ensure the whole of eamxx can follow your clang format ;) ;)
or maybe @mjschmidt271 / @mjs271 (which one is it 👀) might be interested in forcing a formatter?
|
As you might know, we have it in our workflow in MAM4xx repo (I think it was @jeff-cohere's idea). It has been very helpful to keep a consistent formatting. I like the idea of having it for the whole EAMxx, but we must decide that as a group. |
|
@mahf708 - I'd very much be up for looking into this! Dev meeting agenda has been packed lately, but maybe we can throw it to the group tomorrow. And, as of a few weeks ago, trying to make @mjschmidt271 my work-only account... though having varying levels of success so far 🙃 |
|
I could propose something like master...mahf708/eamxx/clang-format (branch https://github.com/E3SM-Project/E3SM/tree/mahf708/eamxx/clang-format), potentially with better hardening of the workflow. Keep in mind we will want to consult with the omega developers as well as infrastructure (this is not an eamxx only change) |
| w0(icol, kk) = 0; | ||
| rho(icol, kk) = -999.0; | ||
| }); | ||
| Kokkos::parallel_for(Kokkos::TeamVectorRange(team, 0u, top_lev), |
There was a problem hiding this comment.
It's interesting that the formatter removes the linebreak here (after parallel_for(), but doesn't do the same in this same file, below...
| rho(icol, kk) = -999.0; | ||
| }); | ||
| Kokkos::parallel_for(Kokkos::TeamVectorRange(team, 0u, top_lev), | ||
| [&](int kk) { |
There was a problem hiding this comment.
This can be a can of worms, since it involves personal preferences, so feel free to ignore.
How do you feel about the indenting here? The lambda gets indented a lot, which can be problematic in cases where the lines are not as short as in this case... Imho, it looks better when it's done like it was before. Or, even better, like this
auto f = [&](int kk) {
...
};
auto tvr = Kokkos::TeamVectorRange(team,0,top_lev);
Kokkos::parallel_for (tvf,f);(but this is not something that a general-puprose formatter can easily do)...
There was a problem hiding this comment.
If you're going to discuss formatting specifics (which we didn't when developing mam4xx), it's helpful to take a look at the (version-dependent) clang-format options that control said specifics.
There was a problem hiding this comment.
I'd personally take the same route @jeff-cohere and company took, just rely on some sane defaults, and then people can work around them in their code if the result ends up ugly (like @bartgol illustrates above)
There was a problem hiding this comment.
Just to add my 2 cents on this, having worked with our clang-format requirement for MAM4xx PRs for a few years now. I can't say I always love the way it makes my code look, but the consistency across the entire project pays dividends on readability since it eliminates idiosyncratic style choices that can provide a constant source of very tiny friction on development.
Also, at this point, I've pretty much internalized and adopted the style so that most times there's little if any intervention by the autoformatter--which is to say, you do get used to it 🙂
|
Thank you all for your input (we also had a discussion on Slack about it in the Infrastructure group channel). At this point, my preference is to merge it (after I resolve the conflict) as is. We can revisit the format issue after we make a project-wide decision. |
mahf708
changed the title
There are no code changes in the PR except for formatting.
[BFB]