diff options
Diffstat (limited to 'docs/contributor_guide/contribution_guidelines.dox')
| -rw-r--r-- | docs/contributor_guide/contribution_guidelines.dox | 89 |
1 files changed, 85 insertions, 4 deletions
diff --git a/docs/contributor_guide/contribution_guidelines.dox b/docs/contributor_guide/contribution_guidelines.dox index f3a6def582..cbaa502635 100644 --- a/docs/contributor_guide/contribution_guidelines.dox +++ b/docs/contributor_guide/contribution_guidelines.dox @@ -1,5 +1,5 @@ /// -/// Copyright (c) 2019-2020 Arm Limited. +/// Copyright (c) 2019-2023 Arm Limited. /// /// SPDX-License-Identifier: MIT /// @@ -35,6 +35,14 @@ The development is structured in the following way: - Development repository: https://review.mlplatform.org/#/admin/projects/ml/ComputeLibrary - Please report issues here: https://github.com/ARM-software/ComputeLibrary/issues +@section S5_0_inc_lang Inclusive language guideline +As part of the initiative to use inclusive language, there are certain phrases and words that were removed or replaced by more inclusive ones. Examples include but not limited to: +\includedoc non_inclusive_language_examples.dox + +Please also follow this guideline when committing changes to Compute Library. +It is worth mentioning that the term "master" is still used in some comments but only in reference to external code links that Arm has no governance on. + +Futhermore, starting from release (22.05), 'master' branch is no longer being used, it has been replaced by 'main'. Please update your clone jobs accordingly. @section S5_1_coding_standards Coding standards and guidelines Best practices (as suggested by clang-tidy): @@ -232,6 +240,18 @@ class ClassName }; @endcode +- In header files, use header guards that use the full file path from the project root and prepend it with "ACL_" + +@code{cpp} +// For File arm_compute/runtime/NEON/functions/NEBatchNormalizationLayer.h +#ifndef ACL_ARM_COMPUTE_RUNTIME_NEON_FUNCTIONS_NEBATCHNORMALIZATIONLAYER +#define ACL_ARM_COMPUTE_RUNTIME_NEON_FUNCTIONS_NEBATCHNORMALIZATIONLAYER +. +. +. +#endif /* ACL_ARM_COMPUTE_RUNTIME_NEON_FUNCTIONS_NEBATCHNORMALIZATIONLAYER */ +@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. @@ -256,6 +276,47 @@ 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 +- When to use const + + - Local variables: Use const as much as possible. E.g. all read-ony variables should be declared as const. + + - Function parameters + + - Top-level const must not be used in the function declaration or definition. (Note that this applies to all types, including non-primitive types) + This is because for function parameters, top-level const in function declaration is always ignored by the compiler (it is meaningless). + Therefore we should omit top-level const to reduce visual clutter. In addition, its omission can improve API/ABI + stability to some extent as there is one fewer varying factor in function signatures. + + Note that we could in theory allow top-level const in only definition (which is not ignored by the compiler) but not declaration. + But certain toolchains are known to require the declaration and definition to match exactly. + + - Use low-level const (of references and pointers) as much as possible. +@code{.cpp} +// Primitive types +void foo(const int a); // NO: Top-level const must not be used in function declaration or definition +void foo(int a); // OK +// Pointer to primitive types +void foo(int *const a); // NO: Top-level const +void foo(const int *const a); // NO: Top-level const +void foo(int *a); // OK. But only if foo needs to mutate the underlying object +void foo(const int *a); // OK but not recommended: See section above about passing primitives by value +// Reference to primitive types +// There's no "top-level const" for references +void foo(int &a); // OK. But only if foo needs to mutate the underlying object +void foo(const int &a); // OK but not recommended: See section above about passing primitives by value + +// Custom types +void foo(const Goo g); // NO: Top-level const +void foo(Goo g); // OK +// Pointer to custom types +void foo(Goo *const g); // NO: Top-level const +void foo(Goo *g); // OK. But only if foo needs to mutate the underlying object +void foo(const Goo *g); // OK +// Reference to custom types +void foo(Goo &g); // OK. But only if foo needs to mutate the underlying object +void foo(const Goo &g); // OK +@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. @@ -264,7 +325,7 @@ auto d = vdup_n_u8(0); // NO: It's not obvious what type this function returns. - 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) + - 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 @@ -274,6 +335,26 @@ auto d = vdup_n_u8(0); // NO: It's not obvious what type this function returns. 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. +We have also provided the python scripts we use in our precommit pipeline inside scripts directory. + - format_code.py: checks Android.bp, bad style, end of file, formats doxygen, runs astyle and clang-format (assuming necessary binaries are in the path). Example invocations: +@code{.sh} + python format_code.py + python format_code.py --error-on-diff + python format_code.py --files=git-diff (Default behavior in pre-commit configuration, where it checks the staged files) +@endcode + - generate_build_files.py: generates build files required for CMake and Bazel builds. Example invocations: +@code{.sh} + python generate_build_files.py --cmake + python generate_build_files.py --bazel +@endcode + +Another way of running the checks is using `pre-commit` (https://pre-commit.com/) framework, which has also nice features like checking trailing spaces, and large committed files etc. +`pre-commit` can be installed via `pip`. After installing, run the following command in the root directory of the repository: + + pre-commit install + +This will create the hooks that perform the formatting checks mentioned above and will automatically run just before committing to flag issues. + @subsection S5_1_3_library_size_guidelines Library size: best practices and guidelines @subsubsection S5_1_3_1_template_suggestions Template suggestions @@ -434,7 +515,7 @@ You can add this to your patch with: You are now ready to submit your patch for review: - git push acl-gerrit HEAD:refs/for/master + git push acl-gerrit HEAD:refs/for/main @section S5_3_code_review Patch acceptance and code review @@ -444,7 +525,7 @@ Once a patch is uploaded for review, there is a pre-commit test that runs on a J - 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. +At the moment, the Jenkins server is not publicly accessible and for security reasons patches submitted by non-allowlisted 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). |