Use static member functions from class instead of instances#25903
Use static member functions from class instead of instances#25903aureleoules wants to merge 2 commits intobitcoin:masterfrom
Conversation
src/crypto/muhash.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i am unfamiliar with spans, how would that work?
There was a problem hiding this comment.
See std::span and our implementation of it Span
There was a problem hiding this comment.
Thanks, attempted in b8f72010a424c48cabc92d8f0e7a43e8eb8460e8.
Not sure if I should add this commit in a separate PR.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
the
2022-08-tidy-use-static-functions
branch
2 times, most recently
from
0ab31bc to
b8f7201
Compare
src/core_write.cpp
Outdated
There was a problem hiding this comment.
personally I prefer ret.npos, or at least think that either is fine and we don't need a linter for this
There was a problem hiding this comment.
dropped e16f6a34b5a200e3ab163dd935f26f17bd1c172f and rolled back to ret.npos instead of std::string::npos in ff9c66a
There was a problem hiding this comment.
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 (#25695 (comment)).
There was a problem hiding this comment.
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
force-pushed
the
2022-08-tidy-use-static-functions
branch
from
b8f7201 to
e9b1c81
Compare
This simplifies the usage of various hasher Write functions as there is no need to pass the buffer size.
aureleoules
force-pushed
the
2022-08-tidy-use-static-functions
branch
from
e9b1c81 to
6f8c291
Compare
|
+1 on switching to Span, -1 on switching from the instance to the class for accessing static methods though. |
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. |
|
Closing because of Concept NACK. |
6 participants
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'.
https://clang.llvm.org/extra/clang-tidy/checks/readability/static-accessed-through-instance.html
I used the
-fixoption of clang-tidy to discover and fix these issues.