-
Notifications
You must be signed in to change notification settings - Fork 435
EAMxx Style Guide #7400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EAMxx Style Guide #7400
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# EAMxx Code Formatting Standards | ||
|
||
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`](https://clang.llvm.org/docs/ClangFormat.html), | ||
and the version we have chosen is v14.[^v14] | ||
<!-- FIXME: Nail this down when figured out --> | ||
- The standard we maintain is largely identical to the | ||
[LLVM Coding Standards](https://llvm.org/docs/CodingStandards.html), | ||
and a list of the handful of customizations of this convention are | ||
enumerated in the configuration file [`$EAMXX_ROOT/.clang-format`](https://github.com/E3SM-Project/E3SM/blob/master/components/eamxx/.clang-format). | ||
- See this [How-to Guide](resources/clang-format_HOWTO.md) | ||
for additional details on how to configure `clang-format` on your chosen | ||
development machine. | ||
|
||
## Automated Workflow Summary | ||
|
||
- The `eamxx-format` workflow runs automatically and passes or fails based on | ||
adherence to our formatting standard. | ||
- The workflow is triggered by any Pull Request (PR) that modifies EAMxx code. | ||
- It is also triggered by other PR-related triggers, such as pushing | ||
changes, or converting from **draft** to **ready**. | ||
- All code modified by a PR must be formatted prior to **merging**. | ||
- It is not necessary for your code to be formatted upon ***opening*** a | ||
Pull Request, but feel free to `clang-format` as you develop if that is | ||
your preference. | ||
- The one situation for which opening a pre-formatted PR may not be | ||
preferred is if the file has never previously been `clang-format`-ed | ||
and requires a large number of changes. | ||
- I.e., touching the majority of lines in a file for format-only | ||
changes will make it difficult for a reviewer to determine which | ||
lines were changed for substantive reasons. | ||
- In this case, please refrain from formatting the code prior to | ||
opening the PR, and, instead, run `clang-format` once the PR is | ||
approved to be merged and make that the final commit. | ||
- As of now, the `eamxx-format` workflow only considers files that are edited | ||
by the Pull Request. | ||
- In addition to the pass/fail status, the workflow, provides the path to any | ||
files that caused the failure. | ||
- This information is found on the ***Summary*** page for any failed | ||
`eamxx-format` ***Job***.[^huh-where] | ||
|
||
[^v14]: It turns out that this is important because there really are | ||
differences in behavior across versions. | ||
[^huh-where]: To get to this summary, select the ***Checks*** tab at the top of | ||
the PR page and select the `eamxx-format` workflow from the left sidebar. | ||
The summary is in the main pane of the page with the title | ||
**clang-format-linter summary**.[^also] | ||
[^also]: Note that this can also be accessed the long way around by following 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
# Functions and Methods | ||
|
||
## Naming | ||
|
||
- Please name functions (methods) to be ***descriptive*** and ***verb**-ish* so | ||
that the name describes what the function *does*. | ||
- 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 general, functions should use `snake_case` and not contain capitalization, | ||
unless capitalizing that quantity is a standard convention (e.g., | ||
`find_Doc_Brown()`). | ||
|
||
## Usage | ||
|
||
- 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. | ||
- As a rule of thumb, developers should prefer passing arguments by reference, | ||
rather than by value, especially in cases for which the argument is ***large*** | ||
or structurally ***complex***. | ||
- E.g., prefer `func(Type &x) { ... }` versus `func(Type x) { ... }`. | ||
- This holds true much of the time because passing by value creates a copy | ||
of the argument for use by the function, and this increases memory | ||
pressure and a resultant performance penalty. | ||
- To be more in-depth on the topic, we echo the guidance from the | ||
[***C++ Core Guidelines***](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-introduction) | ||
and would encourage the curious to read more deeply on the topic. | ||
|
||
> - For "in" parameters, pass cheaply-copied types by value and others by | ||
> reference to `const`. | ||
> - For "in-out" parameters, pass by reference to non-`const`. | ||
> - For "out" parameters, prefer return values to output parameters. | ||
|
||
- The authors go on to explain that "cheap to copy" includes variables | ||
holding 2 or 3 words--for example a double, pointer, or reference. | ||
- To illustrate, the below function adheres to the provided guidance. | ||
|
||
```c++ | ||
SmallStruct func(LargeNComplex ¬Small, double forty_two, | ||
const int *laser, const BigStruct &lotsaData) { | ||
|
||
// SmallStruct object is only assigned-to and then returned, so it is | ||
// defined as the return type | ||
SmallStruct ans; | ||
// forty_two, a scalar double, is considered small and so passed by value | ||
ans.val = forty_two; | ||
// lotsaData, a BigStruct object, is only accessed and so is | ||
// passed by const reference | ||
ans.other_val = lotsaData.small_piece; | ||
|
||
// we pass laser by value and as const because pointers are cheap to copy, | ||
// and the value is not changed | ||
for (int i = 0; i < forty_two; ++i) { | ||
ans.val_vector.push_back(laser[i] + notSmall.epsilon[i]); | ||
} | ||
|
||
// notSmall is large and also modified in the function, so we pass by | ||
// reference | ||
// it is also accessed, above, so it must be an argument and not the | ||
// return value | ||
notSmall.ans_struct = ans; | ||
return ans; | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,208 @@ | ||
# How-to Guide: `clang-format` | ||
|
||
This guide is for developers who wish to apply `clang-format` on their chosen | ||
development machine, whether that be their personal machine or a multi-user | ||
cluster. | ||
In this guide, we will describe how to configure/run `clang-format` on EAMxx | ||
code and also how to install it if necessary. | ||
|
||
## Configure and Run `clang-format` | ||
|
||
Running `clang-format` according to the defined EAMxx standard ***can*** be | ||
done using only command line arguments; however, the command is quite long. | ||
The easier route is to reference the configuration file | ||
(`$EAMXX_ROOT/.clang-format`). | ||
In this case the command is | ||
|
||
```bash {.copy} | ||
clang-format [-i] --style="file:${EAMXX_ROOT}/.clang-format" <path/to/c++/source/file(s)> | ||
``` | ||
|
||
where the `-i` (`--in-place`) argument controls whether the formatting edits | ||
are conducted and change the source file(s) ("in place") or the required edits | ||
are printed to `stdout` (flag omitted). | ||
Also, note that the `--style` flag can also fully define the options without | ||
the configuration file as follows, but this is not recommended as the | ||
configuration could change before this guide is updated to reflect as such. | ||
|
||
<!-- markdownlint-disable MD046 MD013 --> | ||
??? Danger "Terminal One-(long-)liner" | ||
|
||
```bash {.copy} | ||
clang-format [-i] --style="{BasedOnStyle: llvm, ColumnLimit: 100, AlignConsecutiveAssignments: true, AlignConsecutiveBitFields: true, AlignConsecutiveMacros: true, AlignEscapedNewlines: true, AlignTrailingComments: true}" <path/to/c++/source/file(s)> | ||
``` | ||
<!-- markdownlint-enable MD046 MD013 --> | ||
|
||
## Installing `clang-format` | ||
|
||
On a personal machine, which we will consider to be one for which you have | ||
`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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. immediate? it's pretty ez, tell them to get clang-format from conda! |
||
|
||
<!-- markdownlint-disable MD046 --> | ||
??? Note "If you require or already have multiple versions of `clang-format` installed" | ||
|
||
Note that, depending on your requirements, this could be placed in your shell | ||
config file (`.bashrc`, `.zshrc`), or if only needed for a shell session, it | ||
can be done directly from the command line. | ||
|
||
The other option is to add a versioned symbolic link to your `PATH`. | ||
This is sometimes included in the package's `bin/` directory by default and, | ||
if not, can be added there after of placed somewhere that is already on your | ||
`PATH`. | ||
|
||
```c++ | ||
$ cd /opt/homebrew/opt/llvm@14/bin | ||
$ ls clang-format* | ||
clang-format | ||
// no versioned binary so we create symlink | ||
$ ln -s ./clang-format ./clang-format-14 | ||
$ which clang-format | ||
/opt/homebrew/opt/llvm@14/bin/clang-format-14 | ||
``` | ||
|
||
**OR** | ||
|
||
```c++ | ||
$ cd /opt/homebrew/opt/llvm@14/bin | ||
$ ls clang-format* | ||
clang-format | ||
// no versioned binary so we create symlink in a directory already on PATH | ||
$ echo $PATH | ||
/usr/bin:/usr/local/bin | ||
$ ln -s ./clang-format /usr/local/bin/clang-format-14 | ||
$ which clang-format | ||
/usr/local/bin/clang-format-14 | ||
``` | ||
<!-- markdownlint-enable MD046 --> | ||
|
||
=== "Personal Machine" | ||
|
||
<!-- markdownlint-disable MD046 --> | ||
=== "Mac Users (Homebrew Package Manager)" | ||
|
||
For Mac users, the [Homebrew](https://brew.sh) | ||
package manager (`brew`) is the quickest and most straightforward way | ||
to get `clang-format` installed. | ||
Since `clang-format` v14 is not available to install directly from | ||
Homebrew, we install the entire LLVM package at version 14, and this | ||
is as simple as | ||
|
||
```bash {.copy} | ||
brew install llvm@14 | ||
``` | ||
|
||
It it likely that Homebrew will issue a message about not linking the | ||
`clang`-related tools by default, so next we add the binary to our `PATH`. | ||
|
||
```bash {.copy} | ||
$ export PATH="/opt/homebrew/opt/llvm@14/bin/clang-format:$PATH" | ||
# Note: this is the default location for a recent Mac running Apple silicon. | ||
# It may be different on another system. | ||
# You can confirm where yours is installed via 'brew info llvm@14' | ||
$ which clang-format | ||
/opt/homebrew/opt/llvm@14/bin/clang-format | ||
``` | ||
|
||
Note also, that if your system has multiple version of `clang-format` installed, | ||
it may be preferable to instead set a versioned alias to `clang-format` | ||
(`clang-format-14`) as in | ||
|
||
```c++ | ||
// create a shell-alias | ||
alias clang-format-14="/opt/homebrew/opt/llvm@14/bin/clang-format" | ||
``` | ||
|
||
=== "Linux Users (Package Manager)" | ||
|
||
Given the many flavors of Linux, it is difficult to generalize, but there | ||
is a high probability the proper version of `clang-format` or `llvm` is | ||
provided by the built-in package manager. | ||
The commands will differ based on your Linux distribution, but using the | ||
Debian/Ubuntu `apt` syntax, it could be accomplished via something like | ||
|
||
```bash {.copy} | ||
$ apt search clang-format | ||
[...] | ||
clang-format-14/... | ||
$ apt install clang-format-14 | ||
``` | ||
|
||
=== "Build from Source" | ||
|
||
If you do not succeed in the above, `clang-format` can also be fully built | ||
from the [LLVM Compiler Infrastructure](https://github.com/llvm/llvm-project). | ||
It will begin with something like | ||
|
||
```bash {.copy} | ||
git clone git@github.com:llvm/llvm-project.git | ||
git checkout llvmorg-14.0.6 # version tag | ||
``` | ||
|
||
Also, if you only need `clang-format` and not any of the other tooling, | ||
it will build faster/smaller if you use the CMake flag | ||
`-DLLVM_ENABLE_PROJECTS="clang"` to only build `clang` and it's friends, | ||
rather than all of `llvm`. | ||
And finally, the README for [LLVM version 14.0.6](https://github.com/llvm/llvm-project/tree/llvmorg-14.0.6) | ||
is far more comprehensive that the one for the latest version, and it contains | ||
instructions specific to that build. | ||
|
||
=== "Multi-user System" | ||
|
||
In many cases `llvm`, `clang`, or `clang-format` will be available as a module, | ||
though whether version 14 is an option could be less likely. | ||
In the optimistic case, it could be as simple as (using Lmod syntax) | ||
|
||
```bash {.copy} | ||
$ module avail llvm # [clang, clang-format] | ||
[... list ] | ||
$ module load llvm/14.0.6 | ||
``` | ||
|
||
If it is not available, you will probably need to reach out to a system | ||
administrator to get an official version installed.[^but-conda] | ||
<!-- markdownlint-enable MD046 --> | ||
|
||
--- | ||
|
||
<!-- markdownlint-disable MD046 --> | ||
??? Tip "Unnecessary but Convenient Workflow Customization (`direnv`)" | ||
|
||
If you'd like to add a layer of automation/complexity to ensure you only use | ||
`clang-format v14` on `EAMxx` and/or want to use a newer version on the rest | ||
of your system, there is a very handy terminal tool called | ||
[direnv](https://direnv.net/) | ||
(`brew install direnv`) that allows you to automatically load and unload | ||
environment variables based on `$PWD` using a `.envrc` file. | ||
As an example, here's my `.envrc` that adds `clang-format v14` to the path | ||
when I'm working on `EAMxx`. | ||
|
||
```bash {.copy} | ||
PATH_add /opt/homebrew/opt/llvm@14/bin/clang-format | ||
|
||
# also, since I often forget to load a python environment that's required for | ||
# running ctest, this creates or loads a python 3 virtual environment with numpy | ||
layout python3 | ||
pip install --upgrade pip | ||
# the upgrade isn't strictly necessary but trades a little extra setup on | ||
# the front end to avoid pip endlessly reminding you to update | ||
pip install numpy | ||
``` | ||
|
||
This file can be placed in the top-level `EAMxx` directory, and running | ||
`direnv allow` enables the functionality. | ||
Namely, executing `cd EAMxx` loads the defined environment that stays loaded | ||
in any subdirectories, and resets the standard environment when exiting to a | ||
directory above/outside of `EAMxx`. | ||
|
||
For the `conda` fans, this tool can also be used to auto-load a | ||
pre-configured `conda` environment since the `.envrc` is essentially a bash | ||
script with a few bells and whistles tacked on. | ||
<!-- markdownlint-enable MD046 --> | ||
|
||
<!-- markdownlint-disable MD053 --> | ||
[^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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wait-but-what-?!?! we use conda for everything!!!!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) |
||
<!-- markdownlint-enable MD053 --> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
# EAMxx Code Style Standards | ||
|
||
EAMxx does not currently impose strict styling standards, other than those of | ||
the autoformatter. | ||
However, if we can follow consistent style and usage guidelines, we can make | ||
EAMxx developers' lives easier and turnaround times quicker. | ||
In the age of modern text editors with autocomplete and running our code on | ||
machines with enormous storage capacity, there is no real need to save screen | ||
or disk space with minimal naming strategies or other meaning-obscuring style | ||
choices. | ||
So, please adhere to the spirit of these guidelines, and your fellow developers | ||
will thank you for it! | ||
|
||
## General Guidelines | ||
|
||
- Please give ***descriptive***, rather than ***terse***, names to your | ||
variables, functions, classes, etc. | ||
- Avoid idiosyncratic naming or styling conventions. | ||
- If the utility of a style decision would not be immediately apparent to | ||
a reasonable developer, please avoid its usage. | ||
- In the hierarchy of coding concerns, correctness and speed take the top | ||
tiers, but below that level, please favor readability and clarity over | ||
space savings, line-breaking, heavy use of arcane language features, etc. | ||
- That said, if an arcane language feature ***is*** the correct tool to | ||
be used, please do so. | ||
- And perhaps add a comment to aid the non-Jedi-Master developers who | ||
read the code next. :slightly_smiling_face: | ||
- With regard to comments, the general wisdom is that the correct amount of | ||
comments is the amount required to make the code clear and easy to understand. | ||
- To quote | ||
[Jeff Atwood](https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/) | ||
from his essay about code comments: | ||
> Only at the point where the code *cannot* be made easier to understand | ||
should you begin to add comments. | ||
- However, since that does not offer much in the way of prescriptive | ||
guidance, here are some guidelines gathered from around the internet. | ||
- A general rule of thumb, though not always true, is that comments | ||
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.[^dupe] | ||
- Provide links or references when using code from elsewhere or | ||
whenever it is otherwise appropriate. | ||
- A bad comment is worse than no comment. | ||
- 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hahahha, the cursed Quake function--pure poetry 🥇 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good God, what IS that?!? |
||
trick in a complicated interpolation scheme. | ||
[^dupe]: For example, this type of comment does not add any information and | ||
is unnecessary. | ||
|
||
```c++ | ||
// perform initialization | ||
this->initialize(); | ||
``` |
Uh oh!
There was an error while loading. Please reload this page.