feat: clang format & tidy + CI by dbanks12 · Pull Request #393 · AztecProtocol/aztec-packages

dbanks12 · 2023-04-27T14:38:24Z

Description

Blocked by fix: Various manual fixes/tweaks in preparation for clang-tidy PR #415
- Once that ^ PR is merged into master, update this branch

This PR does the following:

Establishes a documented coding standard
Updates VSCode settings to auto-format code on filesave
- uses .clang-format
- includes header grouping & sorting
Adds .clangd settings to provide more VSCode warnings for bad code
Adds CI job to enforce coding standards via clang-tidy
- CI job fails if ./scripts/tidy.sh fix would change code
Tidys entire codebase (.clang-tidy)
Formats entire codebase (updated .clang-format)

Checklist:

I have reviewed my diff in github, line by line.
Every change is related to the PR description.
I have linked this pull request to the issue(s) that it resolves.
There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
The branch has been merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

iAmMichaelConnor · 2023-05-01T09:31:14Z

    auto keccak_hash = ethash_keccak256(reinterpret_cast<uint8_t const*>(func_sig_cstr), strlen(func_sig_cstr));
    // get a pointer to the start of the hash bytes
-    uint8_t const* hash_bytes = reinterpret_cast<uint8_t const*>(&keccak_hash.word64s[0]);
+    auto const* hash_bytes = reinterpret_cast<uint8_t const*>(&keccak_hash.word64s[0]);


Not sure I like that this is being changed from uint8_t to auto, but I can live with it.

Consistent approach throughout, which I got used to, ove the 160 files :D

It seems to only switch to auto when the type is obvious. And that is what I say to do in our standard.

iAmMichaelConnor · 2023-05-01T09:46:04Z


-    Contract<NCT>(std::string const& contract_name)
-        : contract_name(contract_name)
+    explicit Contract<NCT>(std::string const& contract_name) : contract_name(contract_name)


This seems to be the only explicit constructor. I wonder why it chose to make this one explicit (and others not)?

Hm, not sure why it didn't change others too.... I can look into it.

iAmMichaelConnor · 2023-05-01T09:48:50Z

-    static std::vector<Note> UTXO_SLOAD(UTXOSetStateVar<Composer, Note>* utxo_set_state_var,
-                                        size_t const& num_notes,
-                                        typename Note::NotePreimage const& advice);
+    template <typename Note> static std::vector<Note> UTXO_SLOAD(UTXOSetStateVar<Composer, Note>* utxo_set_state_var,


Might be more legible if we always put template <...> on a line before the function declaration. Not worth pauding this PR, though.

We can choose to do have it always on its own line, sometimes on its own line, or just let people use their best judgement. What do you think?

iAmMichaelConnor · 2023-05-01T09:56:50Z


    const NT::address msg_sender = NT::fr(1);
-    const NT::address tx_origin = msg_sender;
+    const NT::address& tx_origin = msg_sender;


Should this be a reference? If msg_sender changes, we don't want tx_origin to change...
Maybe it's ok in this particular case, because tx_origin is probably only used below to initialise structs... and I think (???) it will always create a copy of the value of tx_origin, rather than passing a reference. Makes me nervous, though.

It shouldn't be a problem here because they are both const I believe. But yes you make a good point about it adding & in general. I'll look through and see if there are any sketchy spots where it did this.

iAmMichaelConnor · 2023-05-01T09:58:58Z

-template <typename KernelInput>
-void propagate_valid_state_transitions(KernelInput const& public_kernel_inputs,
-                                       KernelCircuitPublicInputs<NT>& circuit_outputs)
+template <typename KernelInput> void propagate_valid_state_transitions(KernelInput const& public_kernel_inputs,


Again, not sure I like this moving down to be on same line as function declaration. Seems like in some files it does this, in some files it leaves it.

That doesn't seem right. Maybe I have the settings different for functions/structs/enums/classes. I'll look into it.

iAmMichaelConnor

A few comments, but none of those need stop merging

dbanks12 force-pushed the db/clang-format-tidy branch from 998cad7 to 107ece1 Compare April 28, 2023 13:33

iAmMichaelConnor reviewed Apr 28, 2023

View reviewed changes

Comment thread circuits/cpp/src/aztec3/utils/types/circuit_types.hpp

dbanks12 added 9 commits April 28, 2023 22:17

some pre-tidy cleanup

538ab46

cleanup pre-tidy

2081afb

more pre-tidy cleanup

c1d7a9f

pre-tidy fixes

eab57fd

nolint

8067350

add clang files

dfa9d68

CI/docker/tidy files

1d0b58b

full tidy

b5614ec

format

369494f

dbanks12 force-pushed the db/clang-format-tidy branch from 9d1365d to 369494f Compare April 29, 2023 15:34

dbanks12 mentioned this pull request Apr 29, 2023

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

Merged

6 tasks

dbanks12 added 3 commits April 29, 2023 15:48

clang-tidy comment

ffdc04f

Merge branch 'db/clang-format-tidy-setup' into db/clang-format-tidy

8bcf863

coding standard intro and link from readme

374d3be

dbanks12 requested a review from iAmMichaelConnor April 29, 2023 16:11

dbanks12 marked this pull request as ready for review April 29, 2023 16:12

dbanks12 changed the title ~~Draft: clang format & tidy~~ feat: clang format & tidy + CI Apr 29, 2023

merge master in

571d45c

iAmMichaelConnor reviewed May 1, 2023

View reviewed changes

Comment thread circuits/cpp/src/aztec3/circuits/apps/state_vars/mapping_state_var.tpp

iAmMichaelConnor reviewed May 1, 2023

View reviewed changes

Comment thread circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp

iAmMichaelConnor reviewed May 1, 2023

View reviewed changes

Comment thread circuits/cpp/src/aztec3/utils/circuit_errors.hpp

iAmMichaelConnor approved these changes May 1, 2023

View reviewed changes

iAmMichaelConnor merged commit 823979a into master May 1, 2023

iAmMichaelConnor deleted the db/clang-format-tidy branch May 1, 2023 10:10

ludamad pushed a commit that referenced this pull request Jul 14, 2023

Add Keccak constraints to acir_format (#393)

95e33b5

This was referenced Jul 25, 2023

chore(aztec-sandbox): Release 0.2.0-alpha27 #1180

Closed

chore(aztec-sandbox): Release 0.2.0 #1181

Closed

codygunton pushed a commit that referenced this pull request Jan 23, 2024

Add Keccak constraints to acir_format (#393)

8076db9

Conversation

dbanks12 commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iAmMichaelConnor May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iAmMichaelConnor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbanks12 commented Apr 27, 2023 •

edited

Loading

iAmMichaelConnor May 1, 2023 •

edited

Loading

iAmMichaelConnor May 1, 2023 •

edited

Loading