Incremental clang-tidy fixes #213
Conversation
| @@ -406,7 +406,7 @@ int main(int argc, char **argv) | |||
|
|
|||
| #ifdef DEBUG | |||
| // Run only if the flag is set | |||
| if (jr1cs_file != "") { | |||
| if (jr1cs_file.empty()) { | |||
There was a problem hiding this comment.
Was this fixed by the tool? It should be !jr1cs_file.empty()
There was a problem hiding this comment.
That's my mistake. Thanks.
9156300 to
946467d
Compare
1c80995 to
063f1e3
Compare
be2e753 to
a30c182
Compare
a30c182 to
0df760a
Compare
| -modernize-use-auto | ||
| -modernize-use-auto, | ||
| -modernize-use-trailing-return-type, | ||
| -readability-magic-numbers, |
There was a problem hiding this comment.
Do we use "magic numbers" in many places? I like this check actually
There was a problem hiding this comment.
In general it's pretty good, but picks up a few bits of test data and some numbers in expressions (masking expressions, etc) which don't really warrant a whole separate constant. It would be nice if it was a tiny bit less sensitive, so we may want to revisit in the future.
| -readability-magic-numbers, | ||
| -readability-implicit-bool-conversion, | ||
| -modernize-return-braced-init-list, | ||
| -readability-named-parameter, |
There was a problem hiding this comment.
Do we use functions with unnamed arguments?
There was a problem hiding this comment.
Yes, in the case of unused parameters we remove the name to stop the compiler complaining.
| -misc-unused-parameters, | ||
| -readability-redundant-member-init, | ||
| -modernize-deprecated-headers, | ||
| -readability-identifier-naming, |
There was a problem hiding this comment.
I can see that you have disabled many checks here, which is fine (if we can't do otherwise for now). I think it would be nice to add a few lines about why each check has been disabled (as I started to do above) in order to make it crystal clear that we have a reason to remove the checks.
Eg. If we have a lot of magic numbers (in the tests for eg) we can add a line to say "-readability-magic-numbers is removed because we have a lot of magic numbers in the tests [...]". Likewise, I imagine that you removed readability-implicit-bool-conversion because of the boolean vectors we use in the hash implementation, right?
There was a problem hiding this comment.
Added some comments about the disabled checks.
| // mode. See: Section 2.8 https://blake2.net/blake2.pdf Table 2 | ||
| std::array<std::array<bool, BLAKE2s_word_size>, 8> parameter_block = { | ||
| // mode. See: Section 2.8 https://blake2.net/blake2.pdf Table 2. Note, byte | ||
| // endianness has been swapped to avoid doing this repeatedly at runtime. |
There was a problem hiding this comment.
What do you mean by "to avoid doing this repeatedly at runtime"?
There was a problem hiding this comment.
Previously, these constants were endian-swappd before being used (see the .tcc file), and this swap was happening for every invocation of generate_r1cs_witness for every instance of the gadget. Those constants did not seem to be used anywhere else, so the byte swapping seemed unnecessary.
What do you mean by "to avoid doing this repeatedly at runtime"?
It refers to the byte swapping. After this change, it's swapped in the static data, so it doesn't have to be done many times when running the proof gen. (I also put the comment for anyone looking at the reference above, in case they see a discrepency in the order).
There was a problem hiding this comment.
Ah yes, I see. The parameter block structure was taken from Table 2. https://blake2.net/blake2.pdf and since blake processed byte strings as little endian the endianness needed to be switched.
Yes that's fine to switch the endianness directly in the parameter block definition. This is what is done in the RFC (see section 2.5 https://tools.ietf.org/html/rfc7693#page-5)
| /// "Construct" `bits` types from hexadecimal strings | ||
| bits384 bits384_from_hex(const std::string &hex_str); | ||
| template<typename FieldT> | ||
| void fill_variable_array( |
There was a problem hiding this comment.
I would add a comment:
/// Pours the content of the *this - the calling bits object - in the variable "var_array" passed to the funcion
| # | ||
| # -readability-implicit-bool-conversion | ||
| # We use implicit bool conversion for specifying many constants in terms of | ||
| # `0` and `1`. (TODO: it may be a good idea to enable this to fix implicit |
There was a problem hiding this comment.
We can enable this check bu we will need to add a lot of NOLINT comments to disable the checks when we use pb_variable_arrays of booleans (like in blake/sha). This is what I did on #212
Related to #212. Proposal for a subset of warnings to enable for now. Many of the disabled warnings are worth addressing, but may be better done later.