EAMxx Style Guide #7400
EAMxx Style Guide #7400
Conversation
mjschmidt271
requested review from
bartgol,
jgfouca,
AaronDonahue,
tcclevenger and
mahf708
|
| - For example, `install_flux_capacitor()` instead of `f_capacitor_method()` | ||
| or `ifc()`. | ||
| - To note, if your function cannot be named verb-ishly, that is probably a | ||
| sign that there is a fundamental issue with your function. |
| - In situations where multiple class-member variables are passed as function | ||
| arguments, favor passing the object, rather than the individual variables, | ||
| for the sake of shorter lines and function signatures. | ||
| - ==FIXME:== Passing as reference vs. value?? |
There was a problem hiding this comment.
prefer reference, unless for super cheap stuff -- so might as well, just say pass by ref
There was a problem hiding this comment.
Maybe it would be good to mention constness here, since it's a major factor in the treatment of parameters passed to functions.
| `sudo` privileges, installation can be conducted via package manager or by | ||
| building `llvm v14` from scratch. | ||
| If you are a non-admin user of a multi-user cluster, HPC platform, etc., then | ||
| it is likely to be an ***easy*** process, though potentially not immediate. |
There was a problem hiding this comment.
immediate?
it's pretty ez, tell them to get clang-format from conda!
| script with a few bells and whistles tacked on. | ||
|
|
||
| [^but-conda]: There are rumors of using `conda` creatively to do a user-install, | ||
| but that is not an option we support or suggest. |
There was a problem hiding this comment.
wait-but-what-?!?! we use conda for everything!!!!!
There was a problem hiding this comment.
@mahf708 - would you like to add a few lines about doing this with conda?
Happy to add it, if you want to send me a few points of instructions
There was a problem hiding this comment.
No need to add it now. We can revisit later. I am happy with your current version and we can integrate as-is for now :)
| - Add comments to explain non-standard usage or unidiomatic code. | ||
|
|
||
| [^butwhattabout]: An obvious exception to this is explaining complex or opaque | ||
| parts of the code that cannot be made simpler--for instance, a clever arithmetic |
There was a problem hiding this comment.
or a galaxy-brained ones like below
Q_rsqrt
float Q_rsqrt( float number )
{
long i;
float x2, y;
const float threehalfs = 1.5F;
x2 = number * 0.5F;
y = number;
i = * ( long * ) &y; // evil floating point bit level hacking
i = 0x5f3759df - ( i >> 1 ); // what the fuck?
y = * ( float * ) &i;
y = y * ( threehalfs - ( x2 * y * y ) ); // 1st iteration
// y = y * ( threehalfs - ( x2 * y * y ) ); // 2nd iteration, this can be removed
return y;
}There was a problem hiding this comment.
hahahha, the cursed Quake function--pure poetry 🥇
| # You can confirm where yours is installed via 'brew info llvm@14' | ||
| $ which clang-format | ||
| /opt/homebrew/opt/llvm@14/bin/clang-format | ||
| ``` |
There was a problem hiding this comment.
If someone has multiple versions of clang-format on their machine (if they use it for another project, say), clang-format-14 is often provided as a surefire way to call the right one. This can probably be used within workflows to ensure the right version is called.
There was a problem hiding this comment.
good call--thank you!
| To enforce consistent code format throughout EAMxx, we make use of an | ||
| autoformatting workflow, carried out via Github Actions in the E3SM repository. | ||
|
|
||
| - The tool we employ is `clang-format`, and the version we have chosen is |
There was a problem hiding this comment.
Link to https://clang.llvm.org/docs/ClangFormat.html here? It's linked to elsewhere in this guide, so might as well make it available here.
|
|
||
| - Templating and polymorphism are arguably the most powerful features of C++, | ||
| but with great power comes great potential pain. | ||
| - EAMxx makes liberal use of templates, throughout, and this is encouraged |
There was a problem hiding this comment.
You might replace "liberal" with "extensive", since it seems to me that "liberal" and "judicious" are antonyms.
There was a problem hiding this comment.
Ha! Good spot--thanks!
| ``` | ||
|
|
||
| - The first template parameter is an integer-pointer that self-describes. | ||
| - The second template parameters, `HD` (`enum` type), is |
| - If you find yourself unable to name your type this way, that may | ||
| indicate that it should be broken into independent classes, variables, | ||
| or functions. | ||
| - Types should make use of `CamelCase` and begin capitalized |
There was a problem hiding this comment.
UpperCamelCase (not lowerCamelCase)?
There was a problem hiding this comment.
Sorry, I wasn't very clear! This is a trivial wording suggestion. I think you're definitely right about types being capitalized.
| or functions. | ||
| - Types should make use of `CamelCase` and begin capitalized | ||
| (e.g., not `camelCase`). | ||
| - There should be a logical grouping among the variables and classes contained |
There was a problem hiding this comment.
This is not a naming issue, and can get pretty complicated. Maybe it belongs in a "Usage" section?
There was a problem hiding this comment.
Should we also advocate for ordering class members in logical groups? E.g.
- Put all public interface first (ppl more often need to lookup that part of the class when opening the hpp file). There are exceptions, but power users that need exceptions already know that...
- Put methods first, class vars last, for the same reason as above
- Group methods of the same "kind" (e.g., put getters one after the other, and state-changing methods togeter, don't interleave them)
- Put decl of class vars that are similar/connected near each other. E.g., a class storing 2 grids and a bool should prefer the former to the latter in this snippet
class A {
MyGrid source;
MyGrid target;
bool forward;
};
class B {
MyGrid source;
bool forward;
MyGrid target;
};There was a problem hiding this comment.
I like that a lot--will chat with you offline about formalizing this
| [^huh-where]: To get to this summary, follow the ***Actions*** link at the top | ||
| of the E3SM repository page; select `eamxx-format` from the ***All workflows*** | ||
| section of the ***Actions*** sidebar; then choose the most recent run that is | ||
| associated with your PR, which should be near the top of the list. The summary |
There was a problem hiding this comment.
People should also be able to get to the workflow run for their PR from the PR page itself, no?
| should explain ***why*** something is being done in the code, | ||
| rather than ***what*** the code is doing.[^butwhattabout] | ||
| - Comments should dispel confusion and not cause it. | ||
| - Comments should not duplicate the code. |
There was a problem hiding this comment.
Yes. Maybe add an example (or footnote comment) saying that comments like this
// perform initialization
this->initialize();can be avoided as they don't add any info, and they take up lines on the screen. Users should not add redundant comments just to check off a box.
| - Add comments to explain non-standard usage or unidiomatic code. | ||
|
|
||
| [^butwhattabout]: An obvious exception to this is explaining complex or opaque | ||
| parts of the code that cannot be made simpler--for instance, a clever arithmetic |
| - As an example, `AtmosphericManipulator` and not `Manipulator` or `AtMnp`. | ||
| - If you find yourself unable to name your type this way, that may | ||
| indicate that it should be broken into independent classes, variables, | ||
| or functions. |
There was a problem hiding this comment.
Maybe we could point to some small data type in eamxx, to emphasize that even a tiny struct may have a reason to exist, if it makes outer structs smaller/easier to read by encapsulating concepts.
An example could be TimeInterval, which helped me keeping the DataInterpolation class a bit smaller.
| or functions. | ||
| - Types should make use of `CamelCase` and begin capitalized | ||
| (e.g., not `camelCase`). | ||
| - There should be a logical grouping among the variables and classes contained |
There was a problem hiding this comment.
Should we also advocate for ordering class members in logical groups? E.g.
- Put all public interface first (ppl more often need to lookup that part of the class when opening the hpp file). There are exceptions, but power users that need exceptions already know that...
- Put methods first, class vars last, for the same reason as above
- Group methods of the same "kind" (e.g., put getters one after the other, and state-changing methods togeter, don't interleave them)
- Put decl of class vars that are similar/connected near each other. E.g., a class storing 2 grids and a bool should prefer the former to the latter in this snippet
class A {
MyGrid source;
MyGrid target;
bool forward;
};
class B {
MyGrid source;
bool forward;
MyGrid target;
};| - Please name variables as ***descriptive nouns***. | ||
| - That is, for a variable holding data related to *horizontal winds*, choose | ||
| `horizontal_winds` or even `horiz_winds` versus `wh`. | ||
| - In cases for which this may be untenable, add comments explaining |
There was a problem hiding this comment.
Should we add a line with some suggestions on how to mitigate? E.g., don't use articles, or verbs (except is_, can_, has_ prefixes, sometimes), look for synonyms, common word truncations (such as mgr for manager, or dev for device), which can help keep length under control.
Also, how about advocating against removing vowels from words? E.g., we do have cnt for count in a few places, but a) that doesn't shorten much (and often is not necessary), b) reading the code aloud in a public place can be dangerous, and c) making ppl lose 3 seconds parsing an obscure name cost more than the 1 second it cost to scroll an extra line.
| unless capitalizing is a standard convention (e.g., `T` for temperature). | ||
| - We do not currently employ a stylistic distinction between different types of | ||
| variables--e.g., `const`, `static`, member variables, etc. | ||
| - To that end, some developers use a non-standardized convention of |
There was a problem hiding this comment.
I would strongly encourage the reader to embrace one of them though. It does not have to be uniform across the code, but each class should adopt it, unless it's a very small struct. The reason is that when reading a long method, where maybe an input and member have a similar name, you want to know which var represents the obj state:
void A::update_time (double time2)
{
blah blah code
more blah blah code
auto delta = some_func(time); // wait, was time a local var or the class member?
even more blah blah code
}
| that's contained within a class-member struct. | ||
| - This principle applies to calculations as well, as there are situations | ||
| in which readability is improved by breaking complex operations into | ||
| multiple steps. |
There was a problem hiding this comment.
We could add that in non-perf critical code (where readability and bug safe concerns trump everything else) intermediate vars are most likely better, since they are much clearer (unless we're talking about v.length, in which case there's no need for an intermediate var) and help keeping following lines shorter.
housekeeping and fix some broken features changes from PR comments
mjschmidt271
force-pushed
the
mjs/eamxx/docs_style-guide
branch
from
cf2fda4 to
4694e8c
Compare
|
@bartgol - any further changes you'd like on this? |
|
Nope, I think this is good to go. Thanks for working on this. Excellent job. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a draft EAMxx style guide covering code formatting and style standards, including detailed guidelines for types, functions, variables, templating, and additional documentation on using clang-format. In addition, it updates the mkdocs configuration to incorporate a new nested structure for the style guide and removes the legacy style guide file.
- Added new documentation files outlining style, formatting, functions, variables, types, templating, and clang-format instructions.
- Updated navigation in mkdocs to reflect the new nested organization for developer guidelines.
- Removed the legacy developer style guide file to replace it with a more modular structure.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| mkdocs.yaml | Added the "attr_list" markdown extension to improve Markdown attribute parsing. |
| docs/refs/eamxx.bib | Added a set of bibliographic references supporting the documentation. |
| components/eamxx/mkdocs.yml | Revised navigation to include a nested "Style Guide" section with organized subsections. |
| components/eamxx/docs/refs/eamxx.bib | Added additional bibliographic entries related to EAMxx. |
| components/eamxx/docs/refs/aerocom_cldtop.bib | Removed duplicate bibliographic references now provided elsewhere. |
| components/eamxx/docs/developer/style_guide.md | Removed the legacy style guide file in favor of a new modular documentation structure. |
| components/eamxx/docs/developer/style/variables.md | Introduced guidelines on variable naming and usage. |
| components/eamxx/docs/developer/style/types.md | Added detailed guidelines on naming and organizing types, classes, and structs. |
| components/eamxx/docs/developer/style/templates.md | Provided new templating guidelines for C++ code. |
| components/eamxx/docs/developer/style/style_guide_overview.md | Laid out the overview for the updated style guide. |
| components/eamxx/docs/developer/style/style.md | Defined the overall code style standards for EAMxx. |
| components/eamxx/docs/developer/style/resources/clang-format_HOWTO.md | Added a comprehensive guide for installing and configuring clang-format. |
| components/eamxx/docs/developer/style/functions.md | Presented new guidelines on naming and using functions/methods. |
| components/eamxx/docs/developer/style/format.md | Established standards for code formatting practices. |
| components/eamxx/.clang-format | Updated internal comments to clarify how to obtain LLVM style defaults. |
Comments suppressed due to low confidence (1)
components/eamxx/docs/developer/style/functions.md:5
- [nitpick] The markdown emphasis for 'verb-ish' appears inconsistent. Consider using a uniform syntax (for example, verb-ish) to improve clarity.
- Please name functions (methods) to be ***descriptive*** and ***verb**-ish* so that the name describes what the function *does*.
|
I forgot I was the assignee...Integrating now. |
Put together a style guide that covers: Formatting - The clang-format-based autoformatting workflow - General description of the format standard and configuration - Summary of the workflow - How to install/configure/use clang-format on your development machine Style - General guidelines - Specific guidelines for: - Types - Functions/Methods - Variables - Templating
This is a first draft at putting together a style guide that covers:
Formatting
clang-format-based autoformatting workflowclang-formaton your development machineStyle
I'm keeping this a draft for now, as I'd appreciate some feedback on the Style Guide.
In particular:
Note:
This PR also includes some minor tweaks, including fixes to some features (e.g., references) that were broken by the E3SM-level
mkdocs.yml.