[trivial] Switched constants to sizeof() #8321

• Switched a few hardcoded constants to sizeof()
• Broke conditional chains into separate lines with leading conditionals

Broke conditional chains into separate lines with leading conditionals

Our code style is to use the exact opposite... I appreciate your desire to clean up the code base, but I think it touches a lot of lines and thus invalidates any patches of other pulls or patches that developers have in their local tree. Also, I think the cost of review and repository churn is not worth the benefits of this particular pull request.

Agree with @MarcoFalke. Thanks for trying to help, but in general these kinds of pulls which just massage around spaces and newlines aren't well-received. They break other people's patches for no good reason: the difficulty in reading bitcoin core code is not in how the whitespace is organized.

Apologies for missing the section of the codestyle guidelines where it specified trailing conditionals and/or preference for longer lines, I still can't find that part.

So the hardcoded constant to sizeof() conversions are also not seen as worthwhile? It seems that memcpy() with a hardcoded size is asking for trouble down the line.

I still can't find that part.

It is not possible to list all code style preferences. You can use the clang-format-diff.py in the contrib folder if you contribute new code and want to be "on the safe side". Other than that, the code style is usually just defined by the way the local code is written.

So the hardcoded constant to sizeof() conversions are also not seen as worthwhile? It seems that memcpy() with a hardcoded size is asking for trouble down the line.

I think it is not required to use sizeof() when anything other than the hardcoded value would not make sense.

I understand that people are particular about code formatting, and I can even understand the trepidation about branches interfering with each other, despite diff resolution/merging being one of git's strengths.

But never in any project I've contributed to before have I seen someone claim that

memcpy(A, B, <constant>)

is better, preferred, or even equivalent to

memcpy(A, B, sizeof(A))

My hope with a small cleanup branch was to get a feel for the process of submitting to this project before taking on more substantial TODOs, but I get the feeling that is not welcome here.

I'll discard this PR. Thanks for your time.

@tjps Generally you are correct but in this case it is always 32 bytes just by definition. Going to hide this information behind sizeof() would make this less clear. @tjps Don't get me wrong here. Your desire to help is really appreciated but we must keep code-style-only pulls to a minimum as there was nasty bugs introduced in the past by accident.

If you want to get a feel for the process of submitting to this project, we have prepared a list of "easy to do" issues here: https://github.com/bitcoin/bitcoin/labels/Easy%20to%20implement

Note that not all issues are tracked. If you want to help with qa and tests for example, you can ping me to get me compile a list of outstanding improvements.

Also, you can pick a random pull and compile it locally to do see if it does what it is supposed to do. Then report your findings in the pull. Usually you will find a ton of trivial stuff to improve by this. For example: #8336. (Just keep in mind to leave style-only-fixes to a minimum for now)

About the 32 -> sizeof(vch) change... I agree that sizeof(vch) is cleaner, but if a change would be made that changes vch's size, things would break in more subtle ways (because the serialization of keys would differ).

One possibility is doing the 32 -> sizeof(vch), but also adding an asset/static_assert that sizeof(vch) == 32, with a comment explaining that this can't just be changed. That way you get both properties.

OK, how about this as a compromise: I backed out all of the other changes except for the sizeof(), and breaking the conditional chains to one-per-line, which I still assert is a readability win.

I also added a static_assert, which I think is a step better beyond switching to sizeof() - thanks @sipa.

Does this seem reasonable?

It sounds reasonable to me. Concept ack on the sizeof changes only.

@tjps Can you try to squash the commits

git reset c96c1a9~
git commit --verbose -a

@MarcoFalke of course, my mistake. Was late at night.

Hi all, checking in at the one week mark. Is there anything else I can do to get this merged? Hoping to move on to the easy-to-implement list next.

utACK

The changes here are much easier to review with git diff --word-diff than the github output. utACK fbc6070