xref: /external/ComputeLibrary/docs/05_contribution_guidelines.dox

///
/// Copyright (c) 2019 Arm Limited.
///
/// SPDX-License-Identifier: MIT
///
/// Permission is hereby granted, free of charge, to any person obtaining a copy
/// of this software and associated documentation files (the "Software"), to
/// deal in the Software without restriction, including without limitation the
/// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
/// sell copies of the Software, and to permit persons to whom the Software is
/// furnished to do so, subject to the following conditions:
///
/// The above copyright notice and this permission notice shall be included in all
/// copies or substantial portions of the Software.
///
/// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
/// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
/// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
/// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
/// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
/// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
/// SOFTWARE.
///
namespace arm_compute
{
/**
@page contribution_guidelines Contribution guidelines

@tableofcontents

If you want to contribute to Arm Compute Library, be sure to review the following guidelines.

The development is structured in the following way:
- Release repository: https://github.com/arm-software/ComputeLibrary
- Development repository: https://review.mlplatform.org/#/admin/projects/ml/ComputeLibrary
- Please report issues here: https://github.com/ARM-software/ComputeLibrary/issues

@section S5_1_coding_standards Coding standards and guidelines

Best practices (as suggested by clang-tidy):

- No uninitialised values

Helps to prevent undefined behaviour and allows to declare variables const if they are not changed after initialisation. See http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.html

@code{.cpp}
const float32x4_t foo = vdupq_n_f32(0.f);
const float32x4_t bar = foo;

const int32x4x2_t i_foo = {{
	vconvq_s32_f32(foo),
    vconvq_s32_f32(foo)
}};
const int32x4x2_t i_bar = i_foo;
@endcode

- No C-style casts (in C++ source code)

Only use static_cast, dynamic_cast, and (if required) reinterpret_cast and const_cast. See http://en.cppreference.com/w/cpp/language/explicit_cast for more information when to use which type of cast. C-style casts do not differentiate between the different cast types and thus make it easy to violate type safety. Also, due to the prefix notation it is less clear which part of an expression is going to be casted. See http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.html

- No implicit casts to bool

Helps to increase readability and might help to catch bugs during refactoring. See http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-cast.html

@code{.cpp}
extern int *ptr;
if(ptr){} // Bad
if(ptr != nullptr) {} // Good

extern int foo;
if(foo) {} // Bad
if(foo != 0) {} // Good
@endcode

- Use nullptr instead of NULL or 0

The nullptr literal is type-checked and is therefore safer to use. See http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html

- No need to explicitly initialise std::string with an empty string

The default constructor of std::string creates an empty string. In general it is therefore not necessary to specify it explicitly. See http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-init.html

@code{.cpp}
// Instead of
std::string foo("");
std::string bar = "";

// The following has the same effect
std::string foo;
std::string bar;
@endcode

- Braces for all control blocks and loops (which have a body)

To increase readability and protect against refactoring errors the body of control block and loops must be wrapped in braces. See http://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html

For now loops for which the body is empty do not have to add empty braces. This exception might be revoked in the future. Anyway, situations in which this exception applies should be rare.

@code{.cpp}
Iterator it;
while(it.next()); // No need for braces here

// Make more use of it
@endcode

- Only one declaration per line

Increase readability and thus prevent errors.

@code{.cpp}
int a, b; // BAD
int c, *d; // EVEN WORSE

int e = 0; // GOOD
int *p = nullptr; // GOOD
@endcode

- Pass primitive types (and those that are cheap to copy or move) by value

For primitive types it is more efficient to pass them by value instead of by const reference because:

 - the data type might be smaller than the "reference type"
 - pass by value avoids aliasing and thus allows for better optimisations
 - pass by value is likely to avoid one level of indirection (references are often implemented as auto dereferenced pointers)

This advice also applies to non-primitive types that have cheap copy or move operations and the function needs a local copy of the argument anyway.

More information:

 - http://stackoverflow.com/a/14013189
 - http://stackoverflow.com/a/270435
 - http://web.archive.org/web/20140113221447/http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/

@code{.cpp}
void foo(int i, long l, float32x4_t f); // Pass-by-value for builtin types
void bar(const float32x4x4_t &f); // As this is a struct pass-by-const-reference is probably better
void foobar(const MyLargeCustomTypeClass &m); // Definitely better as const-reference except if a copy has to be made anyway.
@endcode

- Don't use unions

Unions cannot be used to convert values between different types because (in C++) it is undefined behaviour to read from a member other than the last one that has been assigned to. This limits the use of unions to a few corner cases and therefor the general advice is not to use unions. See http://releases.llvm.org/3.8.0/tools/clang/tools/extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-union-access.html

- Use pre-increment/pre-decrement whenever possible

In contrast to the pre-incerement the post-increment has to make a copy of the incremented object. This might not be a problem for primitive types like int but for class like objects that overload the operators, like iterators, it can have a huge impact on the performance. See http://stackoverflow.com/a/9205011

To be consistent across the different cases the general advice is to use the pre-increment operator unless post-increment is explicitly required. The same rules apply for the decrement operator.

@code{.cpp}
for(size_t i = 0; i < 9; i++); // BAD
for(size_t i = 0; i < 9; ++i); // GOOD
@endcode

- Don't use uint in C/C++

The C and C++ standards don't define a uint type. Though some compilers seem to support it by default it would require to include the header sys/types.h. Instead we use the slightly more verbose unsigned int type.

- Don't use unsigned int in function's signature

Unsigned integers are good for representing bitfields and modular arithmetic. The fact that unsigned arithmetic doesn't model the behavior of a simple integer, but is instead defined by the standard to model modular arithmetic (wrapping around on overflow/underflow), means that a significant class of bugs cannot be diagnosed by the compiler. Mixing signedness of integer types is responsible for an equally large class of problems.

- No "Yoda-style" comparisons

As compilers are now able to warn about accidental assignments if it is likely that the intention has been to compare values it is no longer required to place literals on the left-hand side of the comparison operator. Sticking to the natural order increases the readability and thus prevents logical errors (which cannot be spotted by the compiler). In the rare case that the desired result is to assign a value and check it the expression has to be surrounded by parentheses.

@code{.cpp}
if(nullptr == ptr || false == cond) // BAD
{
	//...
}

if(ptr == nullptr || cond == false) // GOOD
{
	//...
}

if(ptr = nullptr || cond = false) // Most likely a mistake. Will cause a compiler warning
{
	//...
}

if((ptr = nullptr) || (cond = false)) // Trust me, I know what I'm doing. No warning.
{
	//...
}
@endcode

@subsection S5_1_1_rules Rules

 - Use spaces for indentation and alignment. No tabs! Indentation should be done with 4 spaces.
 - Unix line returns in all the files.
 - Pointers and reference symbols attached to the variable name, not the type (i.e. char \&foo;, and not char& foo).
 - No trailing spaces or tabs at the end of lines.
 - No spaces or tabs on empty lines.
 - Put { and } on a new line and increase the indentation level for code inside the scope (except for namespaces).
 - Single space before and after comparison operators ==, <, >, !=.
 - No space around parenthesis.
 - No space before, one space after ; (unless it is at the end of a line).

@code{.cpp}
for(int i = 0; i < width * height; ++i)
{
	void *d = foo(ptr, i, &addr);
	static_cast<uint8_t *>(data)[i] = static_cast<uint8_t *>(d)[0];
}
@endcode

 - Put a comment after \#else, \#endif, and namespace closing brace indicating the related name

@code{.cpp}
namespace mali
{
#ifdef MALI_DEBUG
	...
#else // MALI_DEBUG
	...
#endif // MALI_DEBUG
} // namespace mali
@endcode

- CamelCase for class names only and lower case words separated with _ (snake_case) for all the functions / methods / variables / arguments / attributes.

@code{.cpp}
class ClassName
{
    public:
        void my_function();
        int my_attribute() const; // Accessor = attribute name minus '_', const if it's a simple type
    private:
        int _my_attribute; // '_' in front of name
};
@endcode

- Use quotes instead of angular brackets to include local headers. Use angular brackets for system headers.
- Also include the module header first, then local headers, and lastly system headers. All groups should be separated by a blank line and sorted lexicographically within each group.
- Where applicable the C++ version of system headers has to be included, e.g. cstddef instead of stddef.h.
- See http://llvm.org/docs/CodingStandards.html#include-style

@code{.cpp}
#include "MyClass.h"

#include "arm_cv/core/Helpers.h"
#include "arm_cv/core/Types.h"

#include <cstddef>
#include <numeric>
@endcode

- Only use "auto" when the type can be explicitly deduced from the assignment.

@code{.cpp}
auto a = static_cast<float*>(bar); // OK: there is an explicit cast
auto b = std::make_unique<Image>(foo); // OK: we can see it's going to be an std::unique_ptr<Image>
auto c = img.ptr(); // NO: Can't tell what the type is without knowing the API.
auto d = vdup_n_u8(0); // NO: It's not obvious what type this function returns.
@endcode

- OpenCL:
    - Use __ in front of the memory types qualifiers and kernel: __kernel, __constant, __private, __global, __local.
    - Indicate how the global workgroup size / offset / local workgroup size are being calculated.

    - Doxygen:

        - No '*' in front of argument names
        - [in], [out] or [in,out] *in front* of arguments
        - Skip a line between the description and params and between params and @return (If there is a return)
        - Align params names and params descriptions (Using spaces), and with a single space between the widest column and the next one.
        - Use an upper case at the beginning of the description

@snippet arm_compute/runtime/NEON/functions/NEActivationLayer.h NEActivationLayer snippet

@subsection S5_1_2_how_to_check_the_rules How to check the rules

astyle (http://astyle.sourceforge.net/) and clang-format (https://clang.llvm.org/docs/ClangFormat.html) can check and help you apply some of these rules.

@subsection S5_1_3_library_size_guidelines Library size: best practices and guidelines

@subsubsection S5_1_3_1_template_suggestions Template suggestions

When writing a new patch we should also have in mind the effect it will have in the final library size. We can try some of the following things:

 - Place non-dependent template code in a different non-templated class/method

@code{.cpp}
template<typename T>
class Foo
{
public:
    enum { v1, v2 };
    // ...
};
@endcode

    can be converted to:

@code{.cpp}
struct Foo_base
{
    enum { v1, v2 };
    // ...
};

template<typename T>
class Foo : public Foo_base
{
public:
    // ...
};
@endcode

 - In some cases it's preferable to use runtime switches instead of template parameters

 - Sometimes we can rewrite the code without templates and without any (significant) performance loss. Let's say that we've written a function where the only use of the templated argument is used for casting:

@code{.cpp}
template <typename T>
void NETemplatedKernel::run(const Window &window)
{
...
 *(reinterpret_cast<T *>(out.ptr())) = *(reinterpret_cast<const T *>(in.ptr()));
...
}
@endcode

The above snippet can be transformed to:

@code{.cpp}
void NENonTemplatedKernel::run(const Window &window)
{
...
std::memcpy(out.ptr(), in.ptr(), element_size);
...
}
@endcode

@subsection S5_1_4_secure_coding_practices Secure coding practices

@subsubsection S5_1_4_1_general_coding_practices General Coding Practices

- **Use tested and approved managed code** rather than creating new unmanaged code for common tasks.
- **Utilize locking to prevent multiple simultaneous requests** or use a synchronization mechanism to prevent race conditions.
- **Protect shared variables and resources** from inappropriate concurrent access.
- **Explicitly initialize all your variables and other data stores**, either during declaration or just before the first usage.
- **In cases where the application must run with elevated privileges, raise privileges as late as possible, and drop them as soon as possible**.
- **Avoid calculation errors** by understanding your programming language's underlying representation and how it interacts with numeric calculation. Pay close attention to byte size discrepancies, precision, signed/unsigned distinctions, truncation, conversion and casting between types, "not-a-number" calculations, and how your language handles numbers that are too large or too small for its underlying representation.
- **Restrict users from generating new code** or altering existing code.


@subsubsection S5_1_4_2_secure_coding_best_practices Secure Coding Best Practices

- **Validate input**. Validate input from all untrusted data sources. Proper input validation can eliminate the vast majority of software vulnerabilities. Be suspicious of most external data sources, including command line arguments, network interfaces, environmental variables, and user controlled files.
- **Heed compiler warnings**. Compile code using the default compiler flags that exist in the SConstruct file.
- Use **static analysis tools** to detect and eliminate additional security flaws.
- **Keep it simple**. Keep the design as simple and small as possible. Complex designs increase the likelihood that errors will be made in their implementation, configuration, and use. Additionally, the effort required to achieve an appropriate level of assurance increases dramatically as security mechanisms become more complex.
- **Default deny**. Base access decisions on permission rather than exclusion. This means that, by default, access is denied and the protection scheme identifies conditions under which access is permitted
- **Adhere to the principle of least privilege**. Every process should execute with the least set of privileges necessary to complete the job. Any elevated permission should only be accessed for the least amount of time required to complete the privileged task. This approach reduces the opportunities an attacker has to execute arbitrary code with elevated privileges.
- **Sanitize data sent to other systems**. Sanitize all data passed to complex subsystems such as command shells, relational databases, and commercial off-the-shelf (COTS) components. Attackers may be able to invoke unused functionality in these components through the use of various injection attacks. This is not necessarily an input validation problem because the complex subsystem being invoked does not understand the context in which the call is made. Because the calling process understands the context, it is responsible for sanitizing the data before invoking the subsystem.
- **Practice defense in depth**. Manage risk with multiple defensive strategies, so that if one layer of defense turns out to be inadequate, another layer of defense can prevent a security flaw from becoming an exploitable vulnerability and/or limit the consequences of a successful exploit. For example, combining secure programming techniques with secure runtime environments should reduce the likelihood that vulnerabilities remaining in the code at deployment time can be exploited in the operational environment.

@subsection S5_1_5_guidelines_for_stable_api_abi Guidelines for stable API/ABI

The Application Programming Interface (API) and Application Binary Interface (ABI) are the interfaces exposed
to users so their programs can interact with the library efficiently and effectively. Even though changing API/ABI
in a way that does not give backward compatibility is not necessarily bad if it can improve other users' experience and the library,
contributions should be made with the awareness of API/ABI stability. If you'd like to make changes that affects
the library's API/ABI, please review and follow the guidelines shown in this section. Also, please note that
these guidelines are not exhaustive list but discussing things that might be easily overlooked.

@subsubsection S5_1_5_1_guidelines_for_api Guidelines for API

- When adding new arguments, consider grouping arguments (including the old ones) into a struct rather than adding arguments with default values.
Introducing a new struct might break the API/ABI once, but it will be helpful to keep the stability.
- When new member variables are added, please make sure they are initialized.
- Avoid adding enum elements in the middle.
- When removing arguments, follow the deprecation process described in the following section.
- When changing behavior affecting API contracts, follow the deprecation process described in the following section.

@subsubsection S5_1_5_2_guidelines_for_abi Guidelines for ABI

We recommend to read through <a href="https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B">this page</a>
and double check your contributions to see if they include the changes listed.

Also, for classes that requires strong ABI stability, consider using <a href="https://en.cppreference.com/w/cpp/language/pimpl">pImpl idiom</a>.

@subsubsection S5_1_5_3_api_deprecation_process API deprecation process

In order to deprecate an existing API, these rules should be followed.

- Removal of a deprecated API should wait at least for one official release.
- Deprecation of runtime APIs should strictly follow the aforementioned period, whereas core APIs can have more flexibility as they are mostly used internally rather than user-facing.
- Any API changes (update, addition and deprecation) in all components should be well documented by the contribution itself.

Also, it is recommended to use the following utility macros which is designed to work with both clang and gcc using C++11 and later.

- ARM_COMPUTE_DEPRECATED: Just deprecate the wrapped function
- ARM_COMPUTE_DEPRECATED_REL: Deprecate the wrapped function and also capture the release that was deprecated
- ARM_COMPUTE_DEPRECATED_REL_REPLACE: Deprecate the wrapped function and also capture the release that was deprecated along with a possible replacement candidate

@code{.cpp}
ARM_COMPUTE_DEPRECATED_REL_REPLACE(20.08, DoNewThing)
void DoOldThing();

void DoNewThing();
@endcode

@section S5_2_how_to_submit_a_patch How to submit a patch

To be able to submit a patch to our development repository you need to have a GitHub account. With that, you will be able to sign in to Gerrit where your patch will be reviewed.

Next step is to clone the Compute Library repository:

	git clone "ssh://<your-github-id>@review.mlplatform.org:29418/ml/ComputeLibrary"

If you have cloned from GitHub or through HTTP, make sure you add a new git remote using SSH:

	git remote add acl-gerrit "ssh://<your-github-id>@review.mlplatform.org:29418/ml/ComputeLibrary"

After that, you will need to upload an SSH key to https://review.mlplatform.org/#/settings/ssh-keys

Then, make sure to install the commit-msg Git hook in order to add a change-ID to the commit message of your patch:

	cd "ComputeLibrary" && mkdir -p .git/hooks && curl -Lo `git rev-parse --git-dir`/hooks/commit-msg https://review.mlplatform.org/tools/hooks/commit-msg; chmod +x `git rev-parse --git-dir`/hooks/commit-msg)

When your patch is ready, remember to sign off your contribution by adding a line with your name and e-mail address to every git commit message:

	Signed-off-by: John Doe <john.doe@example.org>

You must use your real name, no pseudonyms or anonymous contributions are accepted.

You can add this to your patch with:

	git commit -s --amend

You are now ready to submit your patch for review:

	git push acl-gerrit HEAD:refs/for/master

@section S5_3_code_review Patch acceptance and code review

Once a patch is uploaded for review, there is a pre-commit test that runs on a Jenkins server for continuos integration tests. In order to be merged a patch needs to:

- get a "+1 Verified" from the pre-commit job
- get a "+1 Comments-Addressed", in case of comments from reviewers the committer has to address them all. A comment is considered addressed when the first line of the reply contains the word "Done"
- get a "+2" from a reviewer, that means the patch has the final approval

At the moment, the Jenkins server is not publicly accessible and for security reasons patches submitted by non-whitelisted committers do not trigger the pre-commit tests. For this reason, one of the maintainers has to manually trigger the job.

If the pre-commit test fails, the Jenkins job will post a comment on Gerrit with the details about the failure so that the committer will be able to reproduce the error and fix the issue, if any (sometimes there can be infrastructure issues, a test platform disconnecting for example, where the job needs to be retriggered).

*/
} // namespace arm_compute