Skip to content

Conversation

Crypt-iQ
Copy link
Contributor

I noticed the following were unused from the node namespace in validation.cpp:

  • BLOCKFILE_CHUNK_SIZE
  • nPruneTarget
  • OpenBlockFile
  • UNDOFILE_CHUNK_SIZE

I am not sure if maybe there is some reason these are still defined here in which case I'll close this

The following were unused from the node namespace:
- BLOCKFILE_CHUNK_SIZE
- nPruneTarget
- OpenBlockFile
- UNDOFILE_CHUNK_SIZE
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 21, 2022

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

Conflicts

No conflicts as of last run.

@Crypt-iQ
Copy link
Contributor Author

I noticed there is another one in src/rpc/transaction.cpp for ReadBlockFromDisk

@maflcko
Copy link
Member

maflcko commented Jun 23, 2022

not sure if this is worth it. If it is, maybe a iwyu/linter/tidy check would be better?

@Crypt-iQ
Copy link
Contributor Author

I think an automated check would be better than this, but I wouldn't want to add something that rarely catches anything (I think an unused using directive would be kind of rare?). Maybe it's better to let somebody remove these as part of a larger validation PR

@maflcko
Copy link
Member

maflcko commented Jun 24, 2022

We already run clang-tidy, so there is no additional cost of enabling https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html

The question is: Is it worth it?

If the clang-tidy check helps to avoid:

  • pr flood to "fix" this issue
  • review comments to suggest to "fix" this issue
  • wasted review on re-ACKs if the style comment is addressed

I'd say it is worth it.

(Surely it is frustrating to open a pull request and see a red CI 6 minutes later, but if a pull request author isn't willing to put up 6 minutes of their time, maybe they shouldn't open a pull request in the first place to ask others to spend time on it.)

@Crypt-iQ Crypt-iQ closed this Jun 24, 2022
maflcko pushed a commit that referenced this pull request Jul 19, 2022
a02f3f1 tidy: use misc-unused-using-decls (fanquake)
d6787bc refactor: remove unused using directives (fanquake)
3617634 validation: remove unused using directives (eugene)

Pull request description:

  Adds https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html to our clang-tidy.
  PR'd after the discussion in #25433 (which it includes).

ACKs for top commit:
  jamesob:
    Github ACK a02f3f1

Tree-SHA512: 2bb937c1cc90006e69054458d845fb54f287567f4309c773a3fc859f260558c32ff51fc1c2ce9b43207426f3547e7ce226c87186103d741d5efcca19cd355253
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 18, 2022
a02f3f1 tidy: use misc-unused-using-decls (fanquake)
d6787bc refactor: remove unused using directives (fanquake)
3617634 validation: remove unused using directives (eugene)

Pull request description:

  Adds https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html to our clang-tidy.
  PR'd after the discussion in bitcoin#25433 (which it includes).

ACKs for top commit:
  jamesob:
    Github ACK bitcoin@a02f3f1

Tree-SHA512: 2bb937c1cc90006e69054458d845fb54f287567f4309c773a3fc859f260558c32ff51fc1c2ce9b43207426f3547e7ce226c87186103d741d5efcca19cd355253
@bitcoin bitcoin locked and limited conversation to collaborators Jun 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants