From 538ab46842004d0cce6a4771d6e9c0cbd0a60dd7 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 28 Apr 2023 16:12:11 +0000 Subject: [PATCH 1/7] some pre-tidy cleanup --- .../src/aztec3/circuits/abis/private_circuit_public_inputs.hpp | 2 +- .../cpp/src/aztec3/circuits/apps/l1_function_interface.hpp | 2 +- .../src/aztec3/circuits/apps/state_vars/field_state_var.hpp | 2 +- .../src/aztec3/circuits/apps/state_vars/mapping_state_var.hpp | 2 +- .../cpp/src/aztec3/circuits/apps/state_vars/state_var_base.hpp | 2 +- .../cpp/src/aztec3/circuits/apps/state_vars/state_var_base.tpp | 3 +++ .../src/aztec3/circuits/apps/state_vars/utxo_set_state_var.hpp | 2 +- .../cpp/src/aztec3/circuits/apps/state_vars/utxo_state_var.hpp | 2 +- .../aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp | 3 ++- .../cpp/src/aztec3/circuits/rollup/components/components.cpp | 3 ++- circuits/cpp/src/aztec3/oracle/fake_db.hpp | 2 +- 11 files changed, 15 insertions(+), 10 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp b/circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp index 1c3e287bc4e..2873cc75680 100644 --- a/circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp +++ b/circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp @@ -249,7 +249,7 @@ template class OptionalPrivateCircuitPublicInputs { std::optional> contract_deployment_data; - OptionalPrivateCircuitPublicInputs(){}; + OptionalPrivateCircuitPublicInputs() {} OptionalPrivateCircuitPublicInputs(std::optional> const& call_context, diff --git a/circuits/cpp/src/aztec3/circuits/apps/l1_function_interface.hpp b/circuits/cpp/src/aztec3/circuits/apps/l1_function_interface.hpp index b3a9ef6a7e3..c372cca33fb 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/l1_function_interface.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/l1_function_interface.hpp @@ -30,7 +30,7 @@ template class L1FunctionInterface { fr function_selector = 0; size_t num_params = 0; - L1FunctionInterface(){}; + L1FunctionInterface() {} L1FunctionInterface(Contract* contract, L1FunctionInterfaceStruct const& l1_fn_struct) : contract(contract) diff --git a/circuits/cpp/src/aztec3/circuits/apps/state_vars/field_state_var.hpp b/circuits/cpp/src/aztec3/circuits/apps/state_vars/field_state_var.hpp index f16a5315262..acfb262e231 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/state_vars/field_state_var.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/state_vars/field_state_var.hpp @@ -27,7 +27,7 @@ template class FieldStateVar : public StateVar { return *this; } - FieldStateVar(){}; + FieldStateVar() {} // Instantiate a top-level var: FieldStateVar(FunctionExecutionContext* exec_ctx, std::string const& state_var_name, fr const& start_slot) diff --git a/circuits/cpp/src/aztec3/circuits/apps/state_vars/mapping_state_var.hpp b/circuits/cpp/src/aztec3/circuits/apps/state_vars/mapping_state_var.hpp index e697f6e7923..5040f6f9051 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/state_vars/mapping_state_var.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/state_vars/mapping_state_var.hpp @@ -35,7 +35,7 @@ template class MappingStateVar : public StateVar // native_storage_slot.x => value cache, to prevent creating constraints with each `at()` call. std::map value_cache; - MappingStateVar(){}; + MappingStateVar() {} // Instantiate a top-level mapping: MappingStateVar(FunctionExecutionContext* exec_ctx, std::string const& state_var_name) diff --git a/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.hpp b/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.hpp index bcf10f7dbe9..c45beb18105 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.hpp @@ -54,7 +54,7 @@ template class StateVar { // Optionally informs custom notes whether they should commit or partially-commit to this state. bool is_partial_slot = false; - StateVar(){}; + StateVar() {} // Instantiate a top-level state: StateVar(FunctionExecutionContext* exec_ctx, std::string const& state_var_name); diff --git a/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.tpp b/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.tpp index 4592bf55006..6d410cf497b 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.tpp +++ b/circuits/cpp/src/aztec3/circuits/apps/state_vars/state_var_base.tpp @@ -23,8 +23,11 @@ StateVar::StateVar(FunctionExecutionContext* exec_ctx, std:: : exec_ctx(exec_ctx) , state_var_name(state_var_name) { + // NOLINTBEGIN(cppcoreguidelines-prefer-member-initializer) + // this ^ linter rule breaks things here here start_slot = exec_ctx->contract->get_start_slot(state_var_name); storage_slot_point = compute_slot_point(); + // NOLINTEND(cppcoreguidelines-prefer-member-initializer) } template typename CircuitTypes::grumpkin_point StateVar::compute_slot_point() diff --git a/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_set_state_var.hpp b/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_set_state_var.hpp index 1209a3c277e..3a2510bc178 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_set_state_var.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_set_state_var.hpp @@ -34,7 +34,7 @@ template class UTXOSetStateVar : public State typedef typename Note::NotePreimage NotePreimage; - UTXOSetStateVar(){}; + UTXOSetStateVar() {} // Instantiate a top-level var: UTXOSetStateVar(FunctionExecutionContext* exec_ctx, std::string const& state_var_name) diff --git a/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_state_var.hpp b/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_state_var.hpp index 060821fdfaa..900dc0dc979 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_state_var.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/state_vars/utxo_state_var.hpp @@ -36,7 +36,7 @@ template class UTXOStateVar : public StateVar typedef typename Note::NotePreimage NotePreimage; - UTXOStateVar(){}; + UTXOStateVar() {} // Instantiate a top-level var: UTXOStateVar(FunctionExecutionContext* exec_ctx, std::string const& state_var_name) diff --git a/circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp b/circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp index de991ee25c9..6d532b4ed85 100644 --- a/circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp +++ b/circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp @@ -170,7 +170,8 @@ std::array calculate_calldata_hash(BaseRollupInputs const& baseRollup // FIXME // Calculate sha256 hash of calldata; TODO: work out typing here // 22 * 32 = 22 fields, each 32 bytes - std::array calldata_hash_inputs_bytes; + constexpr auto num_bytes = 22 * 32; + std::array calldata_hash_inputs_bytes; // Convert all into a buffer, then copy into the array, then hash for (size_t i = 0; i < calldata_hash_inputs.size(); i++) { auto as_bytes = calldata_hash_inputs[i].to_buffer(); diff --git a/circuits/cpp/src/aztec3/circuits/rollup/components/components.cpp b/circuits/cpp/src/aztec3/circuits/rollup/components/components.cpp index 019072d8cc4..7e100e7b62d 100644 --- a/circuits/cpp/src/aztec3/circuits/rollup/components/components.cpp +++ b/circuits/cpp/src/aztec3/circuits/rollup/components/components.cpp @@ -85,7 +85,8 @@ void assert_equal_constants(DummyComposer& composer, std::array compute_calldata_hash(std::array, 2> previous_rollup_data) { // Generate a 512 bit input from right and left 256 bit hashes - std::array calldata_hash_input_bytes; + constexpr auto num_bytes = 2 * 32; + std::array calldata_hash_input_bytes; for (uint8_t i = 0; i < 2; i++) { std::array calldata_hash_fr = previous_rollup_data[i].base_or_merge_rollup_public_inputs.calldata_hash; diff --git a/circuits/cpp/src/aztec3/oracle/fake_db.hpp b/circuits/cpp/src/aztec3/oracle/fake_db.hpp index c748d050ca6..37825d5c874 100644 --- a/circuits/cpp/src/aztec3/oracle/fake_db.hpp +++ b/circuits/cpp/src/aztec3/oracle/fake_db.hpp @@ -25,7 +25,7 @@ using NT = aztec3::utils::types::NativeTypes; // A temporary stub, whilst building other things first. class FakeDB { public: - FakeDB(){}; + FakeDB() {} /** * For getting a singleton UTXO (not a set). From 2081afbdaba3c97d9ffa0fcb0efdef606045d817 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 28 Apr 2023 22:19:33 +0000 Subject: [PATCH 2/7] cleanup pre-tidy --- circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp | 1 + circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp | 2 +- circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp b/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp index e87bfdeb2ae..a743853f65c 100644 --- a/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp +++ b/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp @@ -61,6 +61,7 @@ template typename PrivatePublic> struct CallStac public_inputs.hash(), }; + // NOLINTNEXTLINE(misc-const-correctness) fr call_stack_item_hash = NCT::compress(inputs, GeneratorIndex::CALL_STACK_ITEM); return call_stack_item_hash; diff --git a/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp b/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp index 5fd86d1dabc..b5393d9075f 100644 --- a/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp +++ b/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp @@ -328,7 +328,7 @@ RootRollupInputs get_root_rollup_inputs(utils::DummyComposer& composer, std::arr * @param initial_values values to pre-populate the tree * @return NullifierMemoryTreeTestingHarness */ -NullifierMemoryTreeTestingHarness get_initial_nullifier_tree(std::vector initial_values) +NullifierMemoryTreeTestingHarness get_initial_nullifier_tree(const std::vector initial_values) { NullifierMemoryTreeTestingHarness nullifier_tree = NullifierMemoryTreeTestingHarness(NULLIFIER_TREE_HEIGHT); for (size_t i = 0; i < initial_values.size(); ++i) { diff --git a/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp b/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp index 97d55314129..bf5dbc405de 100644 --- a/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp +++ b/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp @@ -89,7 +89,7 @@ nullifier_tree_testing_values generate_nullifier_tree_testing_values(BaseRollupI nullifier_tree_testing_values generate_nullifier_tree_testing_values( BaseRollupInputs inputs, std::array new_nullifiers, size_t spacing); -NullifierMemoryTreeTestingHarness get_initial_nullifier_tree(std::vector initial_values); +NullifierMemoryTreeTestingHarness get_initial_nullifier_tree(const std::vector initial_values); KernelData get_empty_kernel(); From c1d7a9f668031d385d6a8d62bad9eaff544b8f82 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 28 Apr 2023 22:35:16 +0000 Subject: [PATCH 3/7] more pre-tidy cleanup --- .../aztec3/circuits/abis/call_stack_item.hpp | 11 +++++---- .../aztec3/circuits/apps/oracle_wrapper.hpp | 23 ++++++++----------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp b/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp index a743853f65c..8171a8086f0 100644 --- a/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp +++ b/circuits/cpp/src/aztec3/circuits/abis/call_stack_item.hpp @@ -1,14 +1,15 @@ #pragma once #include "function_data.hpp" +#include "kernel_circuit_public_inputs.hpp" #include "private_circuit_public_inputs.hpp" #include "public_circuit_public_inputs.hpp" -#include "kernel_circuit_public_inputs.hpp" -#include #include #include #include +#include + namespace aztec3::circuits::abis { using aztec3::utils::types::CircuitTypes; @@ -55,7 +56,7 @@ template typename PrivatePublic> struct CallStac fr hash() const { - std::vector inputs = { + const std::vector inputs = { contract_address.to_field(), function_data.hash(), public_inputs.hash(), @@ -66,7 +67,7 @@ template typename PrivatePublic> struct CallStac return call_stack_item_hash; } -}; // namespace aztec3::circuits::abis +}; // namespace aztec3::circuits::abis template typename PrivatePublic> void read(uint8_t const*& it, CallStackItem& call_stack_item) @@ -96,4 +97,4 @@ std::ostream& operator<<(std::ostream& os, CallStackItem con << "public_inputs: " << call_stack_item.public_inputs << "\n"; } -} // namespace aztec3::circuits::abis \ No newline at end of file +} // namespace aztec3::circuits::abis \ No newline at end of file diff --git a/circuits/cpp/src/aztec3/circuits/apps/oracle_wrapper.hpp b/circuits/cpp/src/aztec3/circuits/apps/oracle_wrapper.hpp index e716ea13ea5..192948a3cb7 100644 --- a/circuits/cpp/src/aztec3/circuits/apps/oracle_wrapper.hpp +++ b/circuits/cpp/src/aztec3/circuits/apps/oracle_wrapper.hpp @@ -2,12 +2,11 @@ #include #include #include - -#include - -#include #include #include +#include + +#include namespace aztec3::circuits::apps { @@ -38,8 +37,7 @@ template class OracleWrapperInterface { // Initialise from Native. // Used when initialising for a user's first call. OracleWrapperInterface(Composer& composer, NativeOracle& native_oracle) - : composer(composer) - , native_oracle(native_oracle){}; + : composer(composer), native_oracle(native_oracle){}; fr& get_msg_sender_private_key() { @@ -96,10 +94,9 @@ template class OracleWrapperInterface { return native_utxo_sload_datum.to_circuit_type(composer); } - template - auto get_utxo_sload_data(grumpkin_point const& storage_slot_point, - size_t const& num_notes, - NotePreimage const& advice) + template auto get_utxo_sload_data(grumpkin_point const& storage_slot_point, + size_t const& num_notes, + NotePreimage const& advice) { auto native_storage_slot_point = aztec3::utils::types::to_nt(storage_slot_point); @@ -120,8 +117,8 @@ template class OracleWrapperInterface { void validate_msg_sender_private_key() { - address msg_sender = get_msg_sender(); - address derived_msg_sender = address::derive_from_private_key(*msg_sender_private_key); + const address msg_sender = get_msg_sender(); + const address derived_msg_sender = address::derive_from_private_key(*msg_sender_private_key); msg_sender.assert_equal(derived_msg_sender, format("msg_sender validation failed.\nmsg_sender_private_key: ", msg_sender_private_key, @@ -132,4 +129,4 @@ template class OracleWrapperInterface { } }; -} // namespace aztec3::circuits::apps \ No newline at end of file +} // namespace aztec3::circuits::apps \ No newline at end of file From eab57fd63c23c3edcd358d0e9b67f3ff6ca1951e Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Sat, 29 Apr 2023 14:13:39 +0000 Subject: [PATCH 4/7] pre-tidy fixes --- .../aztec3/circuits/kernel/public/.test.cpp | 41 ++++++++++--------- .../circuits/rollup/test_utils/utils.hpp | 12 +++--- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp index ea29b73258e..5c7a215461a 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp @@ -1,32 +1,32 @@ #include "init.hpp" #include "native_public_kernel_circuit_no_previous_kernel.hpp" -#include "native_public_kernel_circuit_public_previous_kernel.hpp" #include "native_public_kernel_circuit_private_previous_kernel.hpp" -#include -#include -#include -#include +#include "native_public_kernel_circuit_public_previous_kernel.hpp" #include #include +#include +#include #include #include -#include -#include -#include -#include -#include #include -#include -#include +#include +#include #include #include +#include +#include +#include +#include +#include +#include #include #include -#include #include #include +#include + namespace { using DummyComposer = aztec3::utils::DummyComposer; using aztec3::circuits::abis::public_kernel::PublicKernelInputs; @@ -52,7 +52,7 @@ using aztec3::circuits::apps::FunctionExecutionContext; using aztec3::utils::CircuitErrorCode; using aztec3::utils::source_arrays_are_in_target; using aztec3::utils::zero_array; -} // namespace +} // namespace namespace aztec3::circuits::kernel::public_kernel { @@ -378,12 +378,13 @@ PublicKernelInputs get_kernel_inputs_with_previous_kernel(NT::boolean privat .public_inputs = public_inputs, }; - const PublicKernelInputs kernel_inputs = { + // NOLINTNEXTLINE(misc-const-correctness) + PublicKernelInputs kernel_inputs = { .previous_kernel = previous_kernel, .public_call = kernel_inputs_no_previous.public_call, }; return kernel_inputs; -} // namespace aztec3::circuits::kernel::public_kernel +} // namespace aztec3::circuits::kernel::public_kernel template void validate_public_kernel_outputs_correctly_propagated(const KernelInput& inputs, @@ -743,7 +744,7 @@ TEST(public_kernel_tests, public_kernel_circuit_fails_on_incorrect_msg_sender_in std::array call_stack_hashes; child_call_stacks[0] = generate_call_stack_item(child_contract_address, - contract_address, // this should be the origin_msg_sender, not the contract address + contract_address, // this should be the origin_msg_sender, not the contract address contract_address, contract_portal_address, true, @@ -773,7 +774,7 @@ TEST(public_kernel_tests, public_kernel_circuit_fails_on_incorrect_storage_contr std::array call_stack_hashes; child_call_stacks[0] = generate_call_stack_item(child_contract_address, origin_msg_sender, - child_contract_address, // this should be contract_address + child_contract_address, // this should be contract_address contract_portal_address, true, seed); @@ -804,7 +805,7 @@ TEST(public_kernel_tests, public_kernel_circuit_fails_on_incorrect_portal_contra child_call_stacks[0] = generate_call_stack_item(child_contract_address, origin_msg_sender, contract_address, - child_portal_contract, // this should be contract_portal_address + child_portal_contract, // this should be contract_portal_address true, seed); call_stack_hashes[0] = child_call_stacks[0].hash(); @@ -1056,4 +1057,4 @@ TEST(public_kernel_tests, previous_public_kernel_fails_if_incorrect_storage_cont ASSERT_EQ(dc.get_first_failure().code, CircuitErrorCode::PUBLIC_KERNEL__CALL_CONTEXT_INVALID_STORAGE_ADDRESS_FOR_DELEGATE_CALL); } -} // namespace aztec3::circuits::kernel::public_kernel \ No newline at end of file +} // namespace aztec3::circuits::kernel::public_kernel \ No newline at end of file diff --git a/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp b/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp index bf5dbc405de..062f692d3d7 100644 --- a/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp +++ b/circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.hpp @@ -1,8 +1,10 @@ #pragma once +#include "init.hpp" +#include "nullifier_tree_testing_harness.hpp" + #include "aztec3/circuits/abis/public_data_transition.hpp" + #include "barretenberg/numeric/uint256/uint256.hpp" -#include "nullifier_tree_testing_harness.hpp" -#include "init.hpp" namespace aztec3::circuits::rollup::test_utils::utils { @@ -35,7 +37,7 @@ using aztec3::circuits::abis::MembershipWitness; using aztec3::circuits::abis::PreviousRollupData; using nullifier_tree_testing_values = std::tuple; -} // namespace +} // namespace BaseRollupInputs base_rollup_inputs_from_kernels(std::array kernel_data); @@ -89,7 +91,7 @@ nullifier_tree_testing_values generate_nullifier_tree_testing_values(BaseRollupI nullifier_tree_testing_values generate_nullifier_tree_testing_values( BaseRollupInputs inputs, std::array new_nullifiers, size_t spacing); -NullifierMemoryTreeTestingHarness get_initial_nullifier_tree(const std::vector initial_values); +NullifierMemoryTreeTestingHarness get_initial_nullifier_tree(std::vector initial_values); KernelData get_empty_kernel(); @@ -118,4 +120,4 @@ inline abis::PublicDataRead make_public_read(fr leaf_index, fr value) }; } -} // namespace aztec3::circuits::rollup::test_utils::utils \ No newline at end of file +} // namespace aztec3::circuits::rollup::test_utils::utils \ No newline at end of file From 80673506dabdc011d4c0328ad7b9a80950bbe1ae Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Sat, 29 Apr 2023 14:32:48 +0000 Subject: [PATCH 5/7] nolint --- circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp index 5c7a215461a..b78a9ab0b41 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp @@ -218,6 +218,7 @@ PublicKernelInputsNoPreviousKernel get_kernel_inputs_no_previous_kernel() NT::fr child_portal_contract_address = 200000; std::array call_stack_hashes; for (size_t i = 0; i < PUBLIC_CALL_STACK_LENGTH; i++) { + // NOLINTNEXTLINE(readability-suspicious-call-argument) child_call_stacks[i] = generate_call_stack_item(child_contract_address, contract_address, child_contract_address, @@ -709,6 +710,7 @@ TEST(public_kernel_tests, public_kernel_circuit_succeeds_for_mixture_of_regular_ std::array call_stack_hashes; for (size_t i = 0; i < PUBLIC_CALL_STACK_LENGTH; i++) { child_call_stacks[i] = + // NOLINTNEXTLINE(readability-suspicious-call-argument) generate_call_stack_item(child_contract_address, is_delegate_call ? origin_msg_sender : contract_address, is_delegate_call ? contract_address : child_contract_address, @@ -743,6 +745,7 @@ TEST(public_kernel_tests, public_kernel_circuit_fails_on_incorrect_msg_sender_in NT::fr child_contract_address = 100000; std::array call_stack_hashes; child_call_stacks[0] = + // NOLINTNEXTLINE(readability-suspicious-call-argument) generate_call_stack_item(child_contract_address, contract_address, // this should be the origin_msg_sender, not the contract address contract_address, @@ -802,6 +805,7 @@ TEST(public_kernel_tests, public_kernel_circuit_fails_on_incorrect_portal_contra NT::fr child_contract_address = 100000; NT::fr child_portal_contract = 200000; std::array call_stack_hashes; + // NOLINTNEXTLINE(readability-suspicious-call-argument) child_call_stacks[0] = generate_call_stack_item(child_contract_address, origin_msg_sender, contract_address, From dfa9d684bb7bb4fd970ee046af52310a52ab581a Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Sat, 29 Apr 2023 15:22:53 +0000 Subject: [PATCH 6/7] add clang files --- circuits/cpp/.clang-format | 80 ++++++++++++++-- circuits/cpp/.clang-tidy | 137 +++++++++++++++++++++++++++ circuits/cpp/.clangd | 73 ++++++++++++++ circuits/cpp/.gitignore | 2 - circuits/cpp/scripts/run_tests_local | 1 + circuits/cpp/scripts/tidy.sh | 102 ++++++++++++++++++++ 6 files changed, 385 insertions(+), 10 deletions(-) create mode 100644 circuits/cpp/.clang-tidy create mode 100644 circuits/cpp/.clangd create mode 100755 circuits/cpp/scripts/tidy.sh diff --git a/circuits/cpp/.clang-format b/circuits/cpp/.clang-format index bcd87ae23b2..7e309b5090f 100644 --- a/circuits/cpp/.clang-format +++ b/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 \ No newline at end of file diff --git a/circuits/cpp/.clang-tidy b/circuits/cpp/.clang-tidy new file mode 100644 index 00000000000..a33020b3f22 --- /dev/null +++ b/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-global-names-in-headers, # 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 \ No newline at end of file diff --git a/circuits/cpp/.clangd b/circuits/cpp/.clangd new file mode 100644 index 00000000000..a2fd014d990 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/circuits/cpp/.gitignore b/circuits/cpp/.gitignore index 9811e8181c8..94b21237eb8 100644 --- a/circuits/cpp/.gitignore +++ b/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/* \ No newline at end of file diff --git a/circuits/cpp/scripts/run_tests_local b/circuits/cpp/scripts/run_tests_local index 31269f873e4..298498eaf54 100755 --- a/circuits/cpp/scripts/run_tests_local +++ b/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 diff --git a/circuits/cpp/scripts/tidy.sh b/circuits/cpp/scripts/tidy.sh new file mode 100755 index 00000000000..6399e35e61f --- /dev/null +++ b/circuits/cpp/scripts/tidy.sh @@ -0,0 +1,102 @@ +#!/bin/bash +set -e + +# Run clang-tidy on all C++ source files +# +# CMake must be configured for clang15 before running this script: +# cmake --preset clang15 +# +# Run examples (from circuits/cpp) +# ./scripts/tidy.sh +# or +# ./scripts/tidy.sh fix + +############################################################################### +# ARGS +############################################################################### +# FIX: or fix (MUST BE LOWERCASE) +# if 'fix', apply fixes to actual source file +# else, print warnings +FIX=$1 +# END ARGS +############################################################################### + +# CMake build dir which should contain `compile_commands.json` +BUILD_DIR=build +echo "*************************************************************************" +if [ "$FIX" == "fix" ]; then + echo "Tidy all source files - fix and format the files themselves" + FIX_OPT="-fix -format" +else + EXPORT_DIR=$BUILD_DIR/tidy-fixes + # Clean old tidy fixes + rm -f $EXPORT_DIR/* + mkdir -p $EXPORT_DIR + YAML_FILE=$EXPORT_DIR/tidy-all.yaml + + # run tidy on each source file and export fixes to yaml files + echo "Checking tidy on all source files and exporting fixes to directory: $EXPORT_DIR" + FIX_OPT="-export-fixes $YAML_FILE" + echo "To apply these fixes later, run the following from circuits/cpp:" + echo " clang-apply-replacements --format --style file $EXPORT_DIR" +fi + + +# find all C++ source files to tidy up +SOURCES=$(\ + find src/aztec3/ \ + -name *.cpp \ + -o -name *.hpp \ + -o -name *.cxx \ + -o -name *.hxx \ + -o -name *.tpp \ + -o -name *.cc \ + -o -name *.hh \ + -o -name *.c \ + -o -name *.h \ + | LC_ALL=C sort \ +) + +# MD5 of all source files before running clang-tidy +BEFORE_MD5=$( + for src in ${SOURCES[@]}; do + md5sum $src + done | md5sum) +echo "Before running clang-tidy, MD5 of all C++ files was: $BEFORE_MD5" +echo "*************************************************************************" + +# Need run-clang-tidy version 15, but it doesn't have a --version flag +RUN_TIDY=$(which run-clang-tidy-15 || which run-clang-tidy) +# tidy all sources +$RUN_TIDY -p $BUILD_DIR $SOURCES $FIX_OPT -use-color || {\ + echo "Errors encountered when running clang-tidy!" && + echo "Check the output above before trying again." && \ + exit 1; +} + +echo "*************************************************************************" +if [ "$FIX" == "fix" ]; then + # MD5 of all source files after running clang-tidy + AFTER_MD5=$( + for src in ${SOURCES[@]}; do + md5sum $src + done | md5sum) + echo "AFTER running clang-tidy, MD5 of all C++ files was: $AFTER_MD5" + + echo "$BEFORE_MD5 ?= $AFTER_MD5" + if [ "$BEFORE_MD5" == "$AFTER_MD5" ]; then + echo "No tidying necessary!" + exit 0 + else + echo "WARNING: Some tidying was necessary!" + echo "If you are seeing this in CI, run the following" + echo "in circuits/cpp to tidy up the C++:" + echo " ./scripts/tidy.sh fix" + exit 1 + fi +else + echo "Reminder!" + echo "To apply these fixes later, run the following from circuits/cpp:" + echo " clang-apply-replacements --format --style file $EXPORT_DIR" +fi +echo "*************************************************************************" \ No newline at end of file From ffdc04ffe48a1d1f63d9a3ba315d1f17031e8629 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Sat, 29 Apr 2023 15:48:38 +0000 Subject: [PATCH 7/7] clang-tidy comment --- circuits/cpp/.clang-tidy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/circuits/cpp/.clang-tidy b/circuits/cpp/.clang-tidy index a33020b3f22..a599734df81 100644 --- a/circuits/cpp/.clang-tidy +++ b/circuits/cpp/.clang-tidy @@ -128,7 +128,7 @@ WarningsAsErrors: ' # if disabled, re-enable if fixed in newer clang version # # We should be able to fix: -# -google-global-names-in-headers, # using whole::namespace; is bad +# -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,