Spike: decide on standardization rules for circuits C++

[dbanks12]

Draft of C++ Standards for Aztec Circuits

1. general
   • when something is not covered below, fall back to [Google's style guide](https://google.github.io/styleguide
     /cppguide.html)
   • all TODOs should mention a username, email or bug number

     // TODO(dbanks12)
     // TODO([email protected])
     // TODO(bug 12345)
     

2. spacing
   • 4 spaces except for access-specifiers (public/protected/private) which can use 2
   • namespaces are not indented
   • for continued indentation, just be sane
   • remove trailing spaces at end of line (use a plugin)
   • include a newline at the end of a file

     namespace my_namespace {
     class MyClass {
       public:
         // ... public stuff
     
       protected:
         // ... protected stuff
     
       private:
         // ... private stuff
     
         void my_private_function0(int arg0,
                                   int arg1)
         {
             // ...
         }
     
         void my_private_function1(
             int arg0,
             int arg1)
         {
             // ...
         }
     }
     } // namespace my_namespace
     
     

3. braces
   • functions use curly brace alone on newline
   • namespaces, classes, structs, enums, ifs, and loops use curely brace on same line
   • examples

     void my_function()
     {
         // ...
     }
     
     for (int i = 0; i < max; i++) {
         // ...
     }
     
     if (something_is_true) {
         // ...
     }
     
     struct MyStruct {
         // ...
     }
     

4. naming
   • snake_case for files, namespaces and local variables/members
   • CamelCase for classes, structs, enums, types
     • exceptions types can be made for types if trying to mimic a std type's name like uint or field_ct
   • ALL_CAPS for constexprs, global constants, and macros
   • do use
     • Clear names
     • Descriptive names
   • don't use
     • abbreviations
     • words with letters removed
     • acronyms
     • single letter names
       • Unless writing maths, in which case use your best judgement and follow the naming of a linked paper
5. auto
   • include *, &, and/or const even when using auto
   • use when type is evident
   • use when type should be deduced automatically based on expr
   • use in loops to iterate over members of a container
   • don't use if it makes type unclear
   • don't use you need to enforce a type
   • examples

     auto my_var = my_function_with_unclear_return_type(); // BAD
     auto my_var = get_new_of_type_a(); // GOOD
     

6. const and constexpr
   • use const whenever possible to express immutability
   • use constexpr whenever a const can be computed at compile-time
   • place const/constexpr BEFORE the core type as is done in bberg stdlib
   • examples

     const int my_const = 0;
     constexpr int MY_CONST = 0;
     

7. namespace and using
   • never do using namespace my_namespace; which causes namespace pollution and reduces readability
     • see here for google's corresponding rule
   • namespaces should exactly match directory structure. If you create a nested namespace, create a nested directory for it
   • example for directory aztec3/circuits/abis/private_kernel:

     namespace aztec3::circuits::abis::private_kernel {
         // ...
     } // namespace aztec3::circuits::abis::private_kernel
     

   • useinit.hpp only for core/critical renames like NT/CT and for toggling core types like Composer
   • use unnamed/anonymous namespaces to import and shorten external names into just this one file
     • all of a file's external imports belong in a single anonymous namespace namespace { ...\n } // namespace at the very top of the file directly after #includes
     • use using Rename = old::namespace::prefix::Name; to import and shorten names from external namespaces
     • avoid using renames to obscure template params (using A = A<NT>;)
     • never use renames to remove the std:: prefix
     • never use renames to remove a NT:: or CT:: prefix
   • test.cpp tests must always explicitly import every single name they intend to use
     • they might want to test over multiple namespaces, native and circuit types, and composer types
   • avoid calling barretenberg's functions directly and instead go through interface files like circuit_types and
     native_types
   • using statements should be sorted case according to the LexicographicNumeric rules
     • see the SortUsingDeclarations section of the LLVM Clang Format Style Options document
   • if your IDE is telling you that an include or name is unused in a file, remove it!
8. includes
   • start every header with #pragma once
   • index.hpp should include common headers that will be referenced by most cpp/hpp files in the current directory
   • init.hpp should inject ONLY critical renames (like NT/CT) and type toggles (like Composer)
     • example using NT = aztec3::utils::types::NativeTypes;
   • avoid including headers via relative paths (../../other_dir) unless they are a subdir (subdir/header.hpp)
     • use full path like aztec3/circuits/hash.hpp
   • ordering of includes
     • this source file's header
     • essentials (if present)
       • "index.hpp"
       • "init.hpp"
     • headers nearby
       • headers from this directory (no /)
       • headers from this project using relative path ("private/private_kernel_inputs.hpp")
       • Note: headers in this group are sorted in the above order (no / first, relative paths second)
     • headers from this project using full path (starts with aztec3: "aztec3/constants.hpp")
     • barretenberg headers
     • <gtest> or other third party headers specified in .clang-format
     • C++ standard library headers
   • use quotes internal headers
   • use angle braces for std library and external library headers
     • this includes barretenberg
   • each group of includes should be sorted case sensitive alphabetically
   • each group of includes should be newline-separated
   • example:

     #include "this_file.hpp"
     
     #include "index.hpp"
     #include "init.hpp"
     
     #include "my_file_a_in_this_dir.hpp"
     #include "my_file_b_in_this_dir.hpp"
     #include "other_dir/file_a_nearby.hpp"
     #include "other_dir/file_b_nearby.hpp"
     
     #include "aztec3/file_a_in_project.hpp"
     #include "aztec3/file_b_in_project.hpp"
     
     #include <barretenberg/file_a.hpp>
     #include <barretenberg/file_b.hpp>
     
     #include <gtest>
     
     #include <vector>
     #include <iostream>
     

9. access specifiers
   • order them public, protected, private
10. struct and array initialization
    • use MyStruct my_inst{};
      • will call default constructor if exists and otherwise will value-initialize members to zero (or will call their default constructors if they exist)
    • explicitly initialize struct members with default values: NT::fr my_fr = 0;
    • initialize arrays using std::array<T, 8> my_arr{};
      • this value-initializes all entries to 0 if T has no default constructor, otherwise calls default constructor for each
    • arrays of fields/fr are different! Use zero_array helper function for initialization
      • std::array<fr, VK_TREE_HEIGHT> vk_path = zero_array<fr, VK_TREE_HEIGHT>();
      • This is necessary because fr does have a default constructor that intentionally does not intialize its members to 0
11. references
    • use them whenever possible for function arguments since pass by reference is cheaper
    • make arg references "const" if they should not be modified inside a function
12. avoid C-style coding
    • avoid malloc/free
    • use std::array/vector instead of int[]
    • use references instead of pointers when possible
    • if pointers are necessary, use smart pointers (std::unique_ptr/shared_ptr) instead of raw pointers
    • avoid C-style casts (use static_cast or reinterpret_cast)
13. comments
    • use doxygen docstrings (will include format example)

      /**
       * @brief Brief description
       * @details more details
       * @tparam mytemplateparam description
       * @param myfunctionarg description
       * @return describe return value
       * @see otherRelevantFunction()
       * @see [mylink](url)
       */
      

    • every file should have a meaningful comment
    • every class/struct/function/test should have a meaningful comment
      • class/struct comment might == file comment
    • comment function preconditions ("arg x must be < 100")
14. side-effects
    • avoid functions with side effects when it is easy enough to just have pure functions
    • if a function modifies its arguments, it should be made very clear that this is happening
      • same with class methods that modify members
    • function arguments should be const when they will not be modified
15. global state
    • no
16. docs
    • every subdir should have a readme
17. functions
    • use [[nodiscard]] if it makes no sense to call this function and discard return value

      [[nodiscard] int my_function()
      {
          // ...
          return some_int;
      }
      
      // later can't do
      my_function();
      
      // can only do
      int capture_ret = my_function();
      

    • if there is a name clash, prefix parameter with underscore like _myparam

      void my_function(int _my_var)
      {
          my_var = _my_var;
      }
      

18. macros
    • avoid macros as much as possible with exceptions for
      • testing
      • debug utilities
      • agreed upon macro infrastructure (like cbinds)
19. misc
    • use uintN_t instead of a primitive type (e.g. size_t) when a specific type width must be guaranteed
    • avoid signed types (int, long, char etc) unless signedness is required
      • signed types are susceptible to undefined behavior on overflow/underflow
    • initialize pointers to nullptr
    • constructors with single arguments should be marked explicit to prevent unwanted conversions
    • if a constructor is meant to do nothing, do A() = default; instead of A(){} (explanation here)
      • definitely don't do A(){}; (with semicolon) which can't even be auto-fixed by clang-tidy
    • explicitly use override when overriding a parent class' member (explanation here)
    • avoid multiple declarations on the same line
      • do:

        int a = 0;
        int b = 0;
        

      • dont:

        int a, b = 0, 0;
        

    • use std::vector::emplace_back instead of push_back
      • don't use std::make_pair when using emplace_back (unnecessary as explained here)
    • no magic numbers even if there is a comment explaining them

References

1. Mike's Draft C++ Standard
2. Barretenberg's .clangd
3. Barretenberg's .clang-format
4. LLVM's Clang Format Style Options
5. Google's Style Guide

TODO

1. More examples
2. [In progress] Closely review and compare to barretenberg standards
3. Come up with a preliminary plan for using a linter/formatter for enforcing [subset of] standards
4. Discuss enforcement of preconditions in wasm in lieu of throw created ticket: Spike: Barretenberg ASSERT should not be ignored in wasm, or workaround aztec-packages#308 (and can use DummyComposer in circuits repo generally)
5. Dig into barretenberg_module (to be renamed circuits_module or something similar) and agree on standard for where/when/how modules should be created created ticket: Rename barretenberg_module aztec-packages#364
6. Solicit feedback and refine proposed standards
7. Decide where this standard should live decision: circuits/cpp/CODING_STANDARD.md

[dbanks12]

The actual standardization tickets:

• Standardize usage of auto #219
• Standardize usage of references #220
• Standardize usage of namespaces & includes #190
• Standardize usage of index/init.hpp #221
• Standardize struct and array initialization #215
• Standardize all pointer/malloc usage in c_bind args/returns #206

[dbanks12]

Current plan for linting is to use clang-tidy and clang-format. We will also use the clangd plugin for vscode to enable format-on-save, and to give yellow-squiggles for clang-tidy suggestions. It will not however auto-refactor according to clang-tidy suggestions on save.

We will need to decide whether CI should fully enforce clang-tidy suggestions or not. And should compiler error/warn when tidy has warnings/errors?

[dbanks12]

Draft .clang-format:

Language: Cpp
BasedOnStyle: Google
ColumnLimit: 120

# Whitespace
IndentWidth: 4
UseTab: Never
#LineEnding: LF
MaxEmptyLinesToKeep: 2

# Auto-insertions
#InsertNewlineAtEOF: true
#InsertTrailingCommas: true
#InsertBraces: true

# Alignment
#ReferenceAlignment: Left
PointerAlignment: Left
#QualifierAlignment: Left # only in clang-format 14+

# Misc spacing/linebreaks
AccessModifierOffset: -2
AllowShortFunctionsOnASingleLine: Inline
AllowShortIfStatementsOnASingleLine: Never
AlwaysBreakAfterReturnType: None
AlwaysBreakAfterDefinitionReturnType: None
PenaltyReturnTypeOnItsOwnLine: 1000000
BreakConstructorInitializers: BeforeComma

# Includes
SortIncludes: true # Consider CaseSensitive in clang-format 13+
IncludeBlocks: Regroup
IncludeCategories:
  # Headers in "" with extension.
  - Regex:           '"[A-Za-z]([A-Za-z0-9.\Q/-_\E])+"'
    Priority:        1
  # Headers in "" with extension.
  - Regex:           '"[.]+([A-Za-z0-9.\Q/-_\E])+"'
    Priority:        2
  # Barretenberg headers in ""
  - Regex:           '"barretenberg/.*"'
    Priority:        3
  # Barretenberg headers in <>
  - Regex:           '<barretenberg/.*>'
    Priority:        4
  # Headers in <> with extension.
  - Regex:           '<([A-Za-z0-9.\Q/-_\E])+>'
    Priority:        5
  # Headers in <> from specific external libraries.
  - Regex:           '<(gtest|placeHolderForOthers)\/'
    Priority:        6
  # Headers in <> without extension.
  - Regex:           '<([A-Za-z0-9\Q/-_\E])+>'
    Priority:        7

# Namespaces and using
FixNamespaceComments: true
NamespaceIndentation: None
SortUsingDeclarations: true # LexicographicNumeric

# Bin packing
BinPackArguments: false
BinPackParameters: false

# Braces
Cpp11BracedListStyle: false
#BreakBeforeBraces: Allman
BreakBeforeBraces: Custom
BraceWrapping:
  AfterClass: false
  AfterEnum: false
  AfterFunction: true
  AfterNamespace: false
  AfterStruct: false
  AfterUnion: false
  AfterExternBlock: false
  BeforeCatch: false
  BeforeElse: false
  SplitEmptyFunction: false
  SplitEmptyRecord: false
  SplitEmptyNamespace: false

Draft .clang-tidy:

Checks: '
    cert-*,
    google-*,
    cppcoreguidelines-*,
    readability-*,
    modernize-*,
    bugprone-*,
    misc-*,
    performance-*,
    -misc-const-correctness,
    -modernize-use-trailing-return-type,
    -modernize-avoid-c-arrays,
    -modernize-pass-by-value,
    -modernize-use-nodiscard,
    -cppcoreguidelines-pro-bounds-pointer-arithmetic,
    -readability-magic-numbers,
    -readability-simplify-boolean-expr,
    -cppcoreguidelines-avoid-magic-numbers,
    -readability-identifier-length,
    -bugprone-reserved-identifier,
    -bugprone-easily-swappable-parameters,
    -misc-non-private-member-variables-in-classes,
    -cppcoreguidelines-non-private-member-variables-in-classes,
    -cppcoreguidelines-pro-bounds-constant-array-index,
    -google-build-using-namespace,
    -google-readability-todo,
    -cppcoreguidelines-pro-bounds-array-to-pointer-decay,
    -readability-container-data-pointer,
    -cppcoreguidelines-pro-type-member-init,
    -modernize-use-bool-literals,
    -cert-err58-cpp,
    -cert-dcl37-c,
    -cert-dcl51-cpp,
    -readability-function-cognitive-complexity'
WarningsAsErrors: true
HeaderFilterRegex: 'src/aztec3/.*'
FormatStyle: file

[dbanks12]

Once clang-format/clang-tidy is finalized, each standardization rule should be flagged as covered in the automatic tooling or not.

[iAmMichaelConnor]

Closing because moved to 'Done'