Use static member functions from class instead of instances #25903

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

DrahtBot cross-referenced this on Aug 23, 2022 from issue refactor: Extract RipeMd160 by Empact

DrahtBot cross-referenced this on Aug 23, 2022 from issue crypto: drop 16 byte key support for ChaCha20 by theStack

DrahtBot cross-referenced this on Aug 23, 2022 from issue crypto: avoid potential buffer overread in `ChaCha20::SetKey` by theStack

DrahtBot cross-referenced this on Aug 23, 2022 from issue BIP324: Cipher suite by dhruv

DrahtBot cross-referenced this on Aug 23, 2022 from issue refactor: use std:: prefix for std lib funcs by fanquake

DrahtBot cross-referenced this on Aug 23, 2022 from issue BIP324: Handshake prerequisites by dhruv

DrahtBot cross-referenced this on Aug 23, 2022 from issue BIP324: Add encrypted p2p transport {de}serializer by dhruv

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.

DrahtBot cross-referenced this on Sep 1, 2022 from issue refactor: Use util::Result class for wallet loading by ryanofsky

DrahtBot cross-referenced this on Sep 13, 2022 from issue BIP324: Enable v2 P2P encrypted transport by dhruv

DrahtBot cross-referenced this on Sep 22, 2022 from issue Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa

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