Use static member functions from class instead of instances #25903

pull aureleoules wants to merge 2 commits into bitcoin:master from aureleoules:2022-08-tidy-use-static-functions changing 21 files +40 −24

aureleoules commented at 2:55 PM on August 22, 2022: member

I noticed while reviewing a PR that some static class member functions are accessed by instance instead of class. I believe this makes the code misleading. This PR enables the clang-tidy check 'readability-static-accessed-through-instance'.

Checks for member expressions that access static members through instances, and replaces them with uses of the appropriate qualified-id.

https://clang.llvm.org/extra/clang-tidy/checks/readability/static-accessed-through-instance.html

I used the -fix option of clang-tidy to discover and fix these issues.
w0xlt commented at 3:03 PM on August 22, 2022: contributor

Approach ACK
in src/crypto/muhash.cpp:302 in a1e0710fb2 outdated
```
 298 | @@ -299,7 +299,7 @@ Num3072 MuHash3072::ToNum3072(Span<const unsigned char> in) {
 299 |      unsigned char tmp[Num3072::BYTE_SIZE];
 300 |  
 301 |      uint256 hashed_in{(HashWriter{} << in).GetSHA256()};
 302 | -    ChaCha20(hashed_in.data(), hashed_in.size()).Keystream(tmp, Num3072::BYTE_SIZE);
 303 | +    ChaCha20(hashed_in.data(), uint256::size()).Keystream(tmp, Num3072::BYTE_SIZE);
```
maflcko commented at 3:06 PM on August 22, 2022:

I think the pattern is generally a.data(), a.size(). If you want to change this, it might be best to use spans instead of the changes here in this pull.

aureleoules commented at 3:17 PM on August 22, 2022:

i am unfamiliar with spans, how would that work?

maflcko commented at 3:23 PM on August 22, 2022:

See std::span and our implementation of it Span

aureleoules commented at 9:58 AM on August 23, 2022:

Thanks, attempted in b8f72010a424c48cabc92d8f0e7a43e8eb8460e8. Not sure if I should add this commit in a separate PR.
DrahtBot commented at 1:23 AM on August 23, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
- #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
- #15294 (refactor: Extract RipeMd160 by Empact)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
aureleoules force-pushed on Aug 23, 2022
aureleoules force-pushed on Aug 23, 2022
in src/core_write.cpp:72 in b8f72010a4 outdated
```
  68 | @@ -69,7 +69,7 @@ std::string FormatScript(const CScript& script)
  69 |          ret += strprintf("0x%x ", HexStr(std::vector<uint8_t>(it2, script.end())));
  70 |          break;
  71 |      }
  72 | -    return ret.substr(0, ret.empty() ? ret.npos : ret.size() - 1);
  73 | +    return ret.substr(0, ret.empty() ? std::string::npos : ret.size() - 1);
```
maflcko commented at 6:54 AM on August 24, 2022:

personally I prefer ret.npos, or at least think that either is fine and we don't need a linter for this

aureleoules commented at 12:45 PM on August 25, 2022:

dropped e16f6a34b5a200e3ab163dd935f26f17bd1c172f and rolled back to ret.npos instead of std::string::npos in ff9c66a11d5d2c4dc64d026a0719cb3fc586d17d

maflcko commented at 7:17 AM on August 26, 2022:

I only left a comment on this line, but I meant all other lines as well.

I really don't get the recent rush to enforce meaningless style decisions with clang-tidy, even happily introducing bugs along the way (https://github.com/bitcoin/bitcoin/pull/25695#discussion_r954086171).

aureleoules commented at 11:43 AM on August 26, 2022:

Yes I meant I updated it everywhere as well.

Well, it came from a readability issue and so I thought it would be better to enforce it.

aureleoules commented at 11:46 AM on August 26, 2022:

Happy to drop ff9c66a11d5d2c4dc64d026a0719cb3fc586d17d and keep 6f8c291218041ae843dd9099c7103e607d0f8a39 if needed
aureleoules force-pushed on Aug 25, 2022
refactor: Use static member functions instead of instance ff9c66a11d

crypto: Add Write function overload with Span

This simplifies the usage of various hasher Write functions as there is
no need to pass the buffer size.

6f8c291218

aureleoules force-pushed on Aug 25, 2022
ajtowns commented at 8:27 AM on August 26, 2022: contributor

+1 on switching to Span, -1 on switching from the instance to the class for accessing static methods though.
luke-jr commented at 10:57 PM on August 26, 2022: member

I noticed while reviewing a PR that some static class member functions are accessed by instance instead of class. I believe this makes the code misleading.

I don't agree. Sometimes it makes sense to access by instance instead of class.

If there's particular cases that are confusing, those can be fixed, but I don't think adopting this as a hard rule is a good idea.
aureleoules commented at 8:46 AM on October 21, 2022: member

Closing because of Concept NACK.
aureleoules closed this on Oct 21, 2022
aureleoules deleted the branch on Nov 2, 2022
bitcoin locked this on Nov 2, 2023

Contributors

Use static member functions from class instead of instances #25903

Conflicts