Skip to content

doc: document clang tidy named args #24665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 25, 2022
Merged

doc: document clang tidy named args #24665

merged 2 commits into from
Mar 25, 2022

Conversation

fanquake
Copy link
Member

The documentation, and a single commit extracted from #24661.

Motivation:

Incorrect named args are source of bugs, like #22979.

To allow them being checked by clang-tidy, use a format it can understand.

maflcko
maflcko approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

maflcko
maflcko reviewed Mar 25, 2022
View reviewed changes
src/addrman.cpp
return AddressPosition(/*tried_in=*/false,
/*multiplicity_in=*/addr_info->nRefCount,
/*bucket_in=*/bucket,
/*position_in=*/addr_info->GetBucketPosition(nKey, true, bucket));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated comment:

If msvc was less crappy, we could use designated initializers to enforce named args in constructors without clang-tidy, see #24531

@DrahtBot DrahtBot added the P2P label Mar 25, 2022
@glozow glozow added Docs and removed P2P labels Mar 25, 2022
glozow
glozow reviewed Mar 25, 2022
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, would be nice to be able to point to developer notes when someone asks why I'm formatting arg names this way. The instructions look correct. Were you planning on fixing all of the clang-tidy violations or just these in addrman.cpp?

@fanquake
Copy link
Member Author

Were you planning on fixing all of the clang-tidy violations or just these in addrman.cpp?

and a single commit extracted from #24661.

The rest of the changes are in #24661. That will likely need rebasing a number of times, so I just want to get the docs, and a demonstration commit in for now.

@glozow
Copy link
Member

glozow commented Mar 25, 2022

The rest of the changes are in #24661. That will likely need rebasing a number of times, so I just want to get the docs, and a demonstration commit in for now.

Ok gotchu makes sense

glozow
glozow reviewed Mar 25, 2022
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7e22d80

@fanquake fanquake merged commit 2f0f056 into bitcoin:master Mar 25, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 2, 2022
@fanquake
Merge bitcoin#24665: doc: document clang tidy named args
24a5e0f 
7e22d80 addrman: fix incorrect named args (fanquake)
67f654e doc: Document clang-tidy in dev notes (MarcoFalke)

Pull request description:

  The documentation, and a single commit extracted from bitcoin#24661.

  Motivation:
  > Incorrect named args are source of bugs, like bitcoin#22979.

  > To allow them being checked by clang-tidy, use a format it can understand.

ACKs for top commit:
  glozow:
    ACK 7e22d80

Tree-SHA512: 4037fcea59fdf583b171bce7ad350299fe5f9feb3c398413432168f3b9a185e51884d5b30e4b4ab9c6c5bb896c178cfaee1d78d5b4f0034cd70121c9ea4184b7
@fanquake fanquake deleted the document_clang_tidy branch April 20, 2022 13:37
@bitcoin bitcoin locked and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@glozow glozow glozow left review comments

+1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fanquake @glozow @maflcko @DrahtBot