fix: Various manual fixes/tweaks in preparation for clang-tidy PR (#415)

* some pre-tidy cleanup

* nolint

* add clang files

* clang-tidy comments

circuits/cpp/.clang-format

@@ -1,15 +1,81 @@
-PointerAlignment: Left
+Language: Cpp
+BasedOnStyle: Google
 ColumnLimit: 120
-BreakBeforeBraces: Allman
+
+# Whitespace
 IndentWidth: 4
-BinPackArguments: false
-BinPackParameters: false
-AllowShortFunctionsOnASingleLine: None
-Cpp11BracedListStyle: false
+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
+AlwaysBreakTemplateDeclarations: No
 PenaltyReturnTypeOnItsOwnLine: 1000000
 BreakConstructorInitializers: BeforeComma
+
+# Includes
+SortIncludes: true # Consider CaseSensitive in clang-format 13+
+IncludeBlocks: Regroup
+IncludeCategories:
+  # 1. Special headers
+  - Regex:           '"\(index\.hpp\)|\(init\.hpp\)"'
+    Priority:     1
+  # 2. Headers in "" with no '/' (current dir).
+  - Regex:           '"([A-Za-z0-9.\Q-_\E])+"'
+    Priority:     2
+    SortPriority: 2
+  # 2. Headers in "" that start with '../'.
+  - Regex:           '"\.\./([A-Za-z0-9.\Q-_\E])+"'
+    Priority:     2
+    SortPriority: 4 # same priority/group as 2, but sorted to bottom.
+  # 5. Aztec3 headers in "".
+  - Regex:           '["<]aztec3/([A-Za-z0-9.\Q/-_\E])+[">]'
+    Priority:     5
+  # 6. Barretenberg headers in ""
+  - Regex:           '["<]barretenberg/.*[">]'
+    Priority:     6
+  # 2. All other headers in "".
+  # Note: Must be below aztec3/barretenberg groups or it captures them too.
+  - Regex:           '"([A-Za-z0-9.\Q/-_\E])+"'
+    Priority:     2
+    SortPriority: 3 # same priority/group as 2, but sorted to middle.
+  # 6. Headers in <> with extension.
+  - Regex:           '<([A-Za-z0-9\Q/-_\E])+\.[A-Za-z0-9.\Q/-_\E]>'
+    Priority:     7
+  # 7. Headers in <> from specific external libraries.
+  - Regex:           '<(gtest|placeHolderForOthers)>'
+    Priority:     8
+  # 8. Headers in <> without extension.
+  - Regex:           '<([A-Za-z0-9\Q/-_\E])+>'
+    Priority:     9
+
+# 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
@@ -24,5 +90,3 @@ BraceWrapping:
   SplitEmptyFunction: false
   SplitEmptyRecord: false
   SplitEmptyNamespace: false
-AllowShortFunctionsOnASingleLine : Inline
-SortIncludes: false

circuits/cpp/.clang-tidy

@@ -0,0 +1,137 @@
+# Note there is some overlap between this file and .clangd
+# .clangd has no concept of warnings versus errors, but since it
+# won't actually fail/error (since its really just an LSP for
+# showing issues in an editor), it is acceptable to treat most
+# .clang-tidy warnings as .clangd errors. Also, some error codes
+# with buggy-autofixes need to be omitted from checks in .clang-tidy
+# but can be included in .clangd since it won't change code without
+# a user action.
+#
+# .clang-tidy on the other hand will error in CI for any check that
+# is flagged (not explicitly omitted after '*') in WarningsAsErrors.
+# So, it needs to omit certain checks or keep them as warnings only
+# if they are too strict.
+
+Checks: '
+    cert-*,
+    google-*,
+    cppcoreguidelines-*,
+    readability-*,
+    modernize-*,
+    bugprone-*,
+    misc-*,
+    performance-*,
+    clang-analyzer-*,
+    concurrency-*,
+    portability-*,
+    -bugprone-easily-swappable-parameters,
+    -bugprone-reserved-identifier,
+    -cppcoreguidelines-non-private-member-variables-in-classes,
+    -cppcoreguidelines-pro-bounds-constant-array-index,
+    -cppcoreguidelines-pro-bounds-pointer-arithmetic,
+    -cppcoreguidelines-pro-type-member-init,
+    -cert-dcl37-c,
+    -cert-dcl51-cpp,
+    -cert-dcl59-cpp,
+    -google-build-namespaces,
+    -google-readability-avoid-underscore-in-googletest-name,
+    -google-readability-todo,
+    -misc-non-private-member-variables-in-classes,
+    -modernize-avoid-c-arrays,
+    -modernize-pass-by-value,
+    -modernize-use-nodiscard,
+    -modernize-use-trailing-return-type,
+    -readability-identifier-length,
+    -readability-simplify-boolean-expr,
+    -readability-use-anyofallof,
+'
+
+# We treat all warnings as errors except for these few.
+# Some of these exceptions like 'google-build-using-namespace'
+# we should be able to manually fix project-wide and then
+# remove from the omissions list.
+WarningsAsErrors: '
+    *,
+    -bugprone-unchecked-optional-access,
+    -bugprone-unhandled-self-assignment,
+    -clang-analyzer-core.UndefinedBinaryOperatorResult,
+    -clang-analyzer-cplusplus.NewDeleteLeaks,
+    -clang-analyzer-optin.performance.Padding,
+    -cert-err58-cpp,
+    -cert-oop54-cpp,
+    -cppcoreguidelines-avoid-non-const-global-variables,
+    -cppcoreguidelines-avoid-magic-numbers,
+    -cppcoreguidelines-c-copy-assignment-signature,
+    -cppcoreguidelines-no-malloc,
+    -cppcoreguidelines-owning-memory,
+    -cppcoreguidelines-pro-type-cstyle-cast,
+    -cppcoreguidelines-pro-type-reinterpret-cast,
+    -cppcoreguidelines-special-member-functions,
+    -google-build-using-namespace,
+    -google-global-names-in-headers,
+    -google-readability-casting,
+    -misc-definitions-in-headers,
+    -misc-no-recursion,
+    -misc-unconventional-assign-operator,
+    -modernize-return-braced-init-list,
+    -performance-unnecessary-value-param,
+    -readability-function-cognitive-complexity,
+    -readability-magic-numbers,
+'
+
+# Notes on specific Checks and WarningsAsErrors
+#
+# These checks rename our cbinds and consts that have `__` in the name:
+#    -bugprone-reserved-identifier,
+#    -cert-dcl37-c,
+#    -cert-dcl51-cpp,
+# `any_of/all_of` loops are not objectively more readable:
+#    -readability-use-anyofallof,
+# Will need to refactor our usage of `malloc/free` with `gsl::owner`:
+#    -cppcoreguidelines-owning-memory,
+# Flags way too many functions:
+#    -bugprone-easily-swappable-parameters,
+# We use anon namespaces to import types:
+#    -google-build-namespaces,
+#    -cert-dcl59-cpp,
+# Not sure we want to use trailing return type:
+#    -modernize-use-trailing-return-type,
+# Buggy in clang-tidy 15.0.6:
+#    -modernize-use-nodiscard,
+#    ^ TODO(david): re-enable if we move to newer clang version
+# We use c-arrays in low-level code logic relating to c_bind:
+#    -modernize-avoid-c-arrays,
+# This was changing code in ways that made it harder to follow:
+#    -modernize-pass-by-value,
+#    ^ TODO(david) we probably want to re-enable this one
+# We like (a == true) in circuits:
+#    -readability-simplify-boolean-expr,
+# We have a lot of one-letter variable names...:
+#    -readability-identifier-length,
+# All of our tests use underscores in names:
+#    -google-readability-avoid-underscore-in-googletest-name,
+# All of our circuit structs have non-private members:
+#    -misc-non-private-member-variables-in-classes,
+#    -cppcoreguidelines-non-private-member-variables-in-classes,
+# We have many `for` loops that violate this part of the bounds safety profile
+#    -cppcoreguidelines-pro-bounds-constant-array-index,
+# Many hits potential for false positives:
+#    -cppcoreguidelines-pro-type-member-init,
+# Triggers on some tests that are not complex. We should re-enable this for tests:
+#    -readability-function-cognitive-complexity,
+# Triggers for globals in tests and TEST macros
+#     -cert-err58-cpp,
+# Useful but check is buggy in clang-tidy 15.0.6:
+#    -misc-const-correctness,
+#    ^ TODO(david): re-evaluate whether this one should be included here
+#                   its auto-fixes are buggy
+#                   if disabled, re-enable if fixed in newer clang version
+#
+# We should be able to fix:
+#    -google-build-using-namespace, # using whole::namespace; is bad
+#    -google-readability-todo,  # the auto-fix for this inputs current user's name for all
+#    -cppcoreguidelines-avoid-magic-numbers, # we shouldn't use magic numbers!
+#    -readability-magic-numbers,
+
+HeaderFilterRegex: 'src/aztec3/'
+FormatStyle: file

circuits/cpp/.clangd

@@ -0,0 +1,73 @@
+# Language Server Protocol for showing code problems
+# and suggesting fixes in an editor.
+#
+# See .clang-tidy comments for more information.
+
+CompileFlags:                     # Tweak the parse settings
+  Remove: -fconstexpr-ops-limit=*
+---
+# Applies all barretenberg source files
+If:
+  PathMatch: [src/.*\.hpp, src/.*\.cpp, src/.*\.tcc, src/.*\.h]
+Diagnostics:
+  # Checks whether we are including unused header files
+  # Note that some headers may be _implicitly_ used and still
+  # need to be included, so be careful before removing them.
+  UnusedIncludes: Strict
+
+  # Static analysis configuration
+  ClangTidy:
+    Add:
+      - cert-*
+      - google-*
+      - cppcoreguidelines-*
+      - readability-*
+      - modernize-*
+      - bugprone-*
+      - misc-*
+      - performance-*
+      - clang-analyzer-*
+      - concurrency-*
+      - portability-*
+    Remove:
+      # Fixing this would be a lot of work.
+      - bugprone-easily-swappable-parameters
+      # These checks rename our cbinds and consts that have `__` in the name
+      - bugprone-reserved-identifier
+      # All of our circuit structs have non-private members
+      - cppcoreguidelines-non-private-member-variables-in-classes
+      # We have many `for` loops that violate this part of the bounds safety profile
+      - cppcoreguidelines-pro-bounds-constant-array-index
+      # These checks rename our cbinds and consts that have `__` in the name
+      - cert-dcl37-c
+      - cert-dcl51-cpp
+      # We use anon namespaces to import types
+      - cert-dcl59-cpp
+      # We use anon namespaces to import types
+      - google-build-namespaces
+      # Large diff; we often `use` an entire namespace
+      - google-readability-avoid-underscore-in-googletest-name
+      # All of our circuit structs have non-private members
+      - misc-non-private-member-variables-in-classes
+      # Huge diff. Not sure we want to use trailing return type
+      - modernize-use-trailing-return-type
+      # We like (a == true) in circuits
+      - readability-simplify-boolean-expr
+      # `any_of/all_of` loops are not objectively more readable
+      - readability-use-anyofallof
+
+--- # this divider is necessary
+# Disable some checks for Google Test/Bench
+If:
+  PathMatch: [src/.*\.test\.cpp, src/.*\.bench\.cpp]
+Diagnostics:
+  ClangTidy:
+    # these checks get triggered by the Google macros
+    Remove:
+      # Triggers for globals in tests and TEST macros
+      - cert-err58-cpp
+      - cppcoreguidelines-avoid-non-const-global-variables
+      - cppcoreguidelines-owning-memory
+      - cppcoreguidelines-special-member-functions
+      # Triggers on some tests that are not complex
+      #- readability-function-cognitive-complexity

circuits/cpp/.gitignore

@@ -2,8 +2,6 @@
 build*/
 src/wasi-sdk-*
 CMakeUserPresets.json
-# to be unignored when we agree on clang-tidy rules
-.clangd
 
 # ctest log output dir
 **/Testing/*

circuits/cpp/scripts/run_tests_local

@@ -59,6 +59,7 @@ echo "*** Running $ARCH tests${GTEST_FILTER:+ (with filter: $GTEST_FILTER)}: $TE
 
 cd $BUILD_DIR;
 for BIN in $TESTS; do
+  echo "Running tests from executable '$BIN'"
   if [ "$ARCH" != "wasm" ]; then
     # run test executables directly
     # if gtest filter is non-empty, use it