Skip to content

Commit

Permalink
Update contributing notes
Browse files Browse the repository at this point in the history
  • Loading branch information
@weslleyspereira
weslleyspereira committed Nov 1, 2023
1 parent ddc0d6f commit 744ef99
Showing 1 changed file with 66 additions and 40 deletions.
106 changes: 66 additions & 40 deletions CONTRIBUTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ Thank you for investing your precious time to contribute to \<T\>LAPACK! Please

## Code style

In this section we describe the code style used in \<T\>LAPACK. We also describe the naming conventions used in the library. We suggest following the same style and conventions when contributing to \<T\>LAPACK.

### Automatic code formatting

\<T\>LAPACK uses [ClangFormat](https://clang.llvm.org/docs/ClangFormat.html) (version 10) to enforce a consistent code style. The configuration file is located at [`.clang-format`](.clang-format). The code style is based on the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html) and the differences are marked in the configuration file. You should never modify the [`.clang-format`](.clang-format) file in \<T\>LAPACK, unless this is the reason behind your pull request.
Expand All @@ -12,6 +14,19 @@ To format code you are adding or modifying, we suggest using the [git-clang-form

### Naming conventions

#### General rules

1. Avoid any identifiers starting with underscores even if not followed by Uppercase. This is aligned to [17.4.3.1.2 C++ global names](https://timsong-cpp.github.io/cppwp/n3337/global.names).

> [!NOTE]
> You may use the following regular expression to find relevant identifiers that start with underscore in VS Code:
>
> ```diff
> __(.*)__ # Identifiers limited by double underscore
> ([^\w|])_([A-Z]) # Start with underscore then uppercase letter
> ([^\w])_(\w)([, ;/\)\(.\[\]]) # Other identifiers that start with underscore
> ```
#### Template parameters and type aliases
Template parameters and type aliases should be named using the conventions:
Expand All @@ -36,6 +51,12 @@ Function arguments should be named using either:
For instance, `fooBar` is a good name for a function argument, while `foo_bar` is not. `A` and `matrixA` are good names for a matrix, while `a` is not.
#### Local variables and constants
There is no strict rule for naming local variables. However, we suggest:
1. Declare real-valued constants using real types. For instance, use `const real_type<T> zero(0)` instead of `const T zero(0)`. This is because the type `T` may be a complex type, and the constant `zero` is real. Avoid constants like `const real_type<T> rzero(0)` and `const T czero(0)` in the same function, because it is confusing.
#### Other naming conventions
1. Use upper camel case (Pascal case) style to name:
Expand All @@ -57,6 +78,22 @@ Special cases:
1. Traits are classes with only static members and, thus, they are named using snake case. Moreover, they have the suffix `_trait` or `_traits`, e.g., `tlapack::traits::entry_type_trait` and `tlapack::traits::matrix_type_traits`.
2. Classes for optional arguments of mathematical routines usually have non-static members and, therefore, they are named using upper camel case (Pascal case) style. Moreover, they have the suffix `Opts`, e.g., `tlapack::GeqrfOpts` and `tlapack::BlockedCholeskyOpts`.
## Writing code in the include and src directories
This section describes the guidelines for writing code in the [include](include) and [src](src) directories.
### Return types of computational routines
\<T\>LAPACK routines have three types of return:
1. Routines that return `void`. The void return is reserved for BLAS routines that do not return a value, e.g., `tlapack::scal()` and `tlapack::gemm()`, and for some auxiliary routines like `tlapack::lassq()` and `tlapack::larf()`. Those routines are not supposed to fail and do not need to signal invalid outputs, so they do not return an error code. They could still throw an exception if the input is invalid. See flag `TLAPACK_CHECK_INPUT` in [README.md](README.md#tlapack-options) for more details.
2. Routine that return a value. Those routines are supposed to return a value, e.g., `tlapack::iamax()` and `tlapack::lange()`. The return value is the result of the routine as explained in its documentation.
3. Routines that return an integer. Those routines are supposed to return an error code, e.g., `tlapack::getrf()` and `tlapack::potrf()`. A zero return means that the routine was successful. A non-zero return means that the routine failed to execute and that the output parameters are not valid. The documentation of each routine should specify the significance of each error code.
A routine that fits in the categories 2 and 3, i.e., returns a value and may signal invalid outputs, should have those invalid outputs explicitly in its documentation. For instance, a return 0 in `tlapack::iamax()` will be used to both (1) signal that the input vector is empty and (2) signal that there is a `NAN` at the first position of the input vector.
### Usage of the `auto` keyword
In C++ all types are evaluated at compile time. The `auto` keyword is used to enable the compiler to deduce the type of a variable automatically. For instance, the instructions
Expand All @@ -75,7 +112,7 @@ int n = m + m;

are equivalent, and deduce the type `int` for `n`. We suggest avoiding the usage of `auto` in the following cases:

1. When the type is known, like in [nrm2](include/tlapack/blas/nrm2.hpp):
1. When the type is known, like in [lassq](include/tlapack/lapack/lassq.hpp):

```cpp
const real_t tsml = blue_min<real_t>();
Expand All @@ -88,7 +125,6 @@ are equivalent, and deduce the type `int` for `n`. We suggest avoiding the usage
```cpp
using TB = type_t<matrixB_t>;
using scalar_t = scalar_type<alpha_t,TB>;

const scalar_t alphaTimesblj = alpha*B(l,j);
```
Expand All @@ -108,11 +144,11 @@ We recommend the usage of `auto` in the following cases:

1. To get the output of

a. `tlapack::legacy_matrix` and `tlapack::legacy_vector` when writing wrappers to optimized BLAS and LAPACK.
a. `tlapack::legacy_matrix(...)` and `tlapack::legacy_vector(...)` when writing wrappers to optimized BLAS and LAPACK.

b. slicing matrices and vectors using `tlapack::slice`, `tlapack::rows`, `tlapack::col`, etc. See [concepts](include/tlapack/base/concepts.hpp) for more details.
b. slicing matrices and vectors using `tlapack::slice(...)`, `tlapack::rows(...)`, `tlapack::col(...)`, etc. See [concepts](include/tlapack/base/concepts.hpp) for more details.

c. the functor `tlapack::Create< >(...)`. See [arrayTraits.hpp](include/tlapack/base/arrayTraits.hpp) for more details.
c. the functor `tlapack::Create< >(...)(...)`. See [arrayTraits.hpp](include/tlapack/base/arrayTraits.hpp) for more details.

2. In the return type of functions like `tlapack::asum`, `tlapack::dot`, `tlapack::nrm2` and `tlapack::lange`. By defining the output as `auto`, we enable overloading of those functions using mixed precision. For instance, one may write a overloading of `tlapack::lange` for matrices `Eigen::MatrixXf` that returns `double`.

Expand All @@ -123,30 +159,26 @@ We recommend the usage of `auto` in the following cases:
- check if `x` is zero when calling the constructor `StrongZero(x)`.
- `tlapack_check()`: Used on checks related to the validity of an input of a function. It is enabled if TLAPACK_CHECK_INPUT is defined and TLAPACK_NDEBUG is not. Also used to test the input parameters when creating new legacy matrices and vectors, see `LegacyMatrix.hpp`. THe reason to use `tlapack_check()` instead of assert for matrix and vector creation is to be in the same page as LAPACK. LAPACK routines check the dimensions `m`, `n` and `ldim` the same way it checks other input parameters. `tlapack_check_false(cond)` is the same as `tlapack_check(!cond)`.

### Good practices when writing code inside the library

1. Declare real-valued constants using real types. For instance, use `const real_type<T> zero(0)` instead of `const T zero(0)`. This is because the type `T` may be a complex type, and the constant `zero` is real. Avoid constants like `const real_type<T> rzero(0)` and `const T czero(0)` in the same function, because it is confusing.
### Usage of `constexpr`

2. Avoid any identifiers starting with underscores even if not followed by Uppercase. This is aligned to [17.4.3.1.2 C++ global names](https://timsong-cpp.github.io/cppwp/n3337/global.names). You may use the following regular expression to find relevant identifiers that start with underscore in VS Code:
Use the `constexpr` specifier for:

```diff
__(.*)__ # Identifiers limited by double underscore
([^\w|])_([A-Z]) # Start with underscore then uppercase letter
([^\w])_(\w)([, ;/\)\(.\[\]]) # Other identifiers that start with underscore
```
- Utility functions like `safe_max()`, `abs1()` and `WorkInfo::minMax()`.
- Non-recursive workspace queries.
- Constructors and destructors with simple implementation.

3. In internal calls, use compile-time flags instead of runtime flags. For instance, use `tlapack::LEFT_SIDE` instead of `tlapack::Side::Left` and `tlapack::NO_TRANS` instead of `tlapack::Op::NoTrans`. This practice usually leads to faster code.
### Usage of `inline`

4. Avoid writing code that depends explicitly on `std::complex<T>` by using `tlapack::real_type<T>`, `tlapack::complex_type<T>` and `tlapack::scalar_type<T>`. Any scalar type `T` supported by \<T\>LAPACK should implement those 3 classes.
Use the `inline` specifier on non-template functions implemented on header files. Mind the compilers make the final decision about inlining or not a function, so use it carefully when the objective is performance gain. See https://en.cppreference.com/w/cpp/language/inline. The use of the inline specifier may change the priority for inlining a function by the compiler. It is worth noticing that forcing the inline may lead to large executables, which is specially bad when the library is already based on templates.

5. Use the `constexpr` specifier for:
### Good practices

- Utility functions like `safe_max()`, `abs1()` and `WorkInfo::minMax()`.
- Non-recursive workspace queries.
1. In internal calls, use compile-time flags instead of runtime flags. For instance, use `tlapack::LEFT_SIDE` instead of `tlapack::Side::Left` and `tlapack::NO_TRANS` instead of `tlapack::Op::NoTrans`. This practice usually leads to faster code.

6. Use the `inline` specifier on non-template functions implemented on header files. Mind the compilers make the final decision about inlining or not a function, so use it carefully when the objective is performance gain. See https://en.cppreference.com/w/cpp/language/inline. The use of the inline specifier may change the priority for inlining a function by the compiler. It is worth noticing that forcing the inline may lead to large executables, which is specially bad when the library is already based on templates.
2. Avoid writing code that depends explicitly on `std::complex<T>` by using `tlapack::real_type<T>`, `tlapack::complex_type<T>` and `tlapack::scalar_type<T>`. Any scalar type `T` supported by \<T\>LAPACK should implement those 3 classes.

> **_NOTE:_** `std::complex` is one way to define complex numbers. It has undesired behavior such as:
> [!NOTE]
> `std::complex` is one way to define complex numbers. It has undesired behavior such as:
>
> - `std::abs(std::complex<T>)` may not propagate NaNs. See https://github.com/tlapack/tlapack/issues/134#issue-1364091844.
> - Operations with `std::complex<T>`, for `T=float,double,long double` are wrappers to operations in C. Other types have their implementation in C++. Because of that, the logic of complex multiplication, division and other operations may change from type to type. See https://github.com/advanpix/mpreal/issues/11.
Expand Down Expand Up @@ -186,7 +218,7 @@ Consider following the steps below before writing a test for a new routine:

- If you need to create a matrix, use `tlapack::Create<TestType>(...)`. See [arrayTraits.hpp](include/tlapack/base/arrayTraits.hpp) for more details.

- If you need to create a vector there are two options. The first option is to use `tlapack::Create< vector_type<TestType> >(...)`. See [arrayTraits.hpp](include/tlapack/base/arrayTraits.hpp) for more details. The secon optionis to use `std::vector< type_t<TestType> >` or `std::vector< real_type<type_t<TestType>> >`.
- If you need to create a vector there are two options. The first option is to use `tlapack::Create< vector_type<TestType> >(...)`. See [arrayTraits.hpp](include/tlapack/base/arrayTraits.hpp) for more details. The second option is to use `std::vector< type_t<TestType> >` or `std::vector< real_type<type_t<TestType>> >`.

6. Use the macro `GENERATE()` to create a range of values. For instance, you may use `GENERATE(1,2,3)` to create a range of values `{1,2,3}`. This way you can avoid writing a loop to test a routine for different values of a parameter.

Expand Down Expand Up @@ -214,18 +246,7 @@ Consider following the steps below before writing a test for a new routine:
TO-DO
## Other topics

### Updating [tests/blaspp](tests/blaspp) and [tests/lapackpp](tests/lapackpp)

There are two situations in which you may need to update [tests/blaspp](tests/blaspp) and [tests/lapackpp](tests/lapackpp):

1. When you want to enable a test.
2. When you want to use a new version of those libraries for tests.

### Running Github Actions locally

You can run the Github Actions locally using [act](https://github.com/nektos/act). This is useful for debugging the Github Actions workflow.
## Writing documentation
### Using `@todo` to mark incomplete code
Expand All @@ -238,14 +259,19 @@ You can use `@todo` to mark incomplete code. For instance, some code may work fo

Using the triple slash `///` with tag `@todo` will make sure that the reminder is visible in the Doxygen documentation.

### Return types of \<T\>LAPACK routines
## Writing examples

\<T\>LAPACK routines have three types of return:
TO-DO

1. Routines that return `void`. The void return is reserved for BLAS routines that do not return a value, e.g., `tlapack::scal()` and `tlapack::gemm()`, and for some auxiliary routines like `tlapack::lassq()` and `tlapack::larf()`. Those routines are not supposed to fail and do not need to signal invalid outputs, so they do not return an error code. They could still throw an exception if the input is invalid. See flag `TLAPACK_CHECK_INPUT` in [README.md](README.md#tlapack-options) for more details.
## Other topics

2. Routine that return a value. Those routines are supposed to return a value, e.g., `tlapack::iamax()` and `tlapack::lange()`. The return value is the result of the routine as explained in its documentation.
### Updating [tests/blaspp](tests/blaspp) and [tests/lapackpp](tests/lapackpp)

3. Routines that return an integer. Those routines are supposed to return an error code, e.g., `tlapack::getrf()` and `tlapack::potrf()`. A zero return means that the routine was successful. A non-zero return means that the routine failed to execute and that the output parameters are not valid. The documentation of each routine should specify the significance of each error code.
There are two situations in which you may need to update [tests/blaspp](tests/blaspp) and [tests/lapackpp](tests/lapackpp):

A routine that fits in the categories 2 and 3, i.e., returns a value and may signal invalid outputs, should have those invalid outputs explicitly in its documentation. For instance, a return 0 in `tlapack::iamax()` will be used to both (1) signal that the input vector is empty and (2) signal that there is a `NAN` at the first position of the input vector.
1. When you want to enable a test.
2. When you want to use a new version of those libraries for tests.

### Running Github Actions locally

You can run the Github Actions locally using [act](https://github.com/nektos/act). This is useful for debugging the Github Actions workflow.

0 comments on commit 744ef99

Please sign in to comment.