Skip to content
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

compat: document code in compat.h #25493

Merged
merged 8 commits into from
Jul 20, 2022
Merged

compat: document code in compat.h #25493

merged 8 commits into from
Jul 20, 2022

Conversation

fanquake
Copy link
Member

Move compat.h into compat/, and document what is in there.

@fanquake fanquake force-pushed the document_compat_code branch 3 times, most recently from f558168 to ae2db1c Compare June 28, 2022 17:11
@jamesob
Copy link
Contributor

jamesob commented Jun 28, 2022

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 29, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

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.

This was referenced Jun 29, 2022
Merged
Closed
vasild
vasild approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK ae2db1c

src/random.cpp Outdated Show resolved Hide resolved
src/compat/compat.h Show resolved Hide resolved
@fanquake fanquake force-pushed the document_compat_code branch from ae2db1c to 5cb0589 Compare June 29, 2022 11:05
vasild
vasild approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 5cb0589

@fanquake
Copy link
Member Author

We will need to update the minisketch subtree, to accomodate the compat.h renaming, as the minisketch header, in the subtree, actually includes compat.h, when building for MSVC.

@fanquake fanquake force-pushed the document_compat_code branch from 5cb0589 to a43777b Compare June 29, 2022 13:48
@fanquake
Copy link
Member Author

For now I've rebased this on top of a minisketch subtree update, and a cherry-pick of sipa/minisketch#66; that way we'll get a run against the MSVC CI. I've also added a commit to use the SSIZE_T change in our code.

Once that PR is merged, I'll open a PR to update the subtree in our repo. Once that is merged, I'll rebase this.

@fanquake fanquake mentioned this pull request Jun 29, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Jun 29, 2022
Merged
@fanquake fanquake mentioned this pull request Jun 29, 2022
Merged
achow101 added a commit that referenced this pull request Jun 29, 2022
@achow101
Merge #25502: upstream: update minisketch subtree
5bc10b3 
28a28a0 Squashed 'src/minisketch/' changes from 7eeb778fef..47f0a2d26f (fanquake)

Pull request description:

  Contains:
  * sipa/minisketch#65
  * sipa/minisketch#66

  Required for #25493.

ACKs for top commit:
  achow101:
    ACK dc375e5
  hebasto:
    ACK dc375e5, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: fbcd6cdc551770ff67d1df65ab171ce43c9eb7e7668da76da5c5b06865ed550527abcff661741a86c1535018a85a165619ff94ae3e6c7a695374b6c4f843c5ca
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 29, 2022
@achow101
Merge bitcoin#25502: upstream: update minisketch subtree
89439cd 
28a28a0 Squashed 'src/minisketch/' changes from 7eeb778fef..47f0a2d26f (fanquake)

Pull request description:

  Contains:
  * sipa/minisketch#65
  * sipa/minisketch#66

  Required for bitcoin#25493.

ACKs for top commit:
  achow101:
    ACK dc375e5
  hebasto:
    ACK dc375e5, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: fbcd6cdc551770ff67d1df65ab171ce43c9eb7e7668da76da5c5b06865ed550527abcff661741a86c1535018a85a165619ff94ae3e6c7a695374b6c4f843c5ca
@fanquake fanquake force-pushed the document_compat_code branch from a43777b to 18ce5f9 Compare June 30, 2022 08:08
@fanquake fanquake marked this pull request as ready for review June 30, 2022 08:08
@DrahtBot DrahtBot mentioned this pull request Jul 1, 2022
Closed
@fanquake fanquake requested a review from vasild July 4, 2022 16:45
vasild
vasild approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 18ce5f9

luke-jr
luke-jr reviewed Jul 5, 2022
View reviewed changes
src/compat/compat.h Outdated Show resolved Hide resolved
@fanquake fanquake force-pushed the document_compat_code branch from 18ce5f9 to bdbdfe0 Compare July 6, 2022 10:13
vasild
vasild approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK bdbdfe0

@fanquake fanquake force-pushed the document_compat_code branch from bdbdfe0 to 122c242 Compare July 13, 2022 10:14
vasild
vasild approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 122c242

@DrahtBot DrahtBot mentioned this pull request Jul 13, 2022
Closed
@fanquake fanquake requested review from laanwj, jamesob and hebasto July 13, 2022 12:24
hebasto
hebasto approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 122c242

The S_IRUSR and S_IWUSR macro are used in src/wallet/bdb.cpp which requires to #include <compat/compat.h>. Maybe #include <sys/stat.h> could be moved from src/wallet/bdb.cpp into compat/compat.h as well.

@fanquake fanquake force-pushed the document_compat_code branch from 122c242 to 56470a1 Compare July 20, 2022 09:47
@fanquake
Copy link
Member Author

Maybe #include <sys/stat.h> could be moved from src/wallet/bdb.cpp into compat/compat.h as well.

Rebased and incorporated this change.

@vasild
Copy link
Contributor

vasild commented Jul 20, 2022 •
edited
Loading

I would expect non-standard headers to be included in compat.h (inside some #ifdef magic) and then other source files to include compat.h. This implies that a non-standard header is included only in compat.h. sys/stat.h is included in 5 files (in httpserver.cpp without #ifdef 👀). Should all of those be replaced with #include <compat.h>?

compat.h is included in 27 files. Now all of them (indirectly) include sys/stat.h.

The commit message of 3a865ea compat: document S_I* defines when building for Windows needs to be extended because now the commit does more than mentioned.

@fanquake fanquake force-pushed the document_compat_code branch from 56470a1 to f7dc992 Compare July 20, 2022 12:11
@fanquake
Copy link
Member Author

Ok. I've just reverted Hebastos suggestion for now. If someone wants to go an massage the headers later (with IWYU), they can. However doing that is unrelated to adding documentation.

vasild
vasild approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK f7dc992

hebasto
hebasto approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK f7dc992

@maflcko maflcko merged commit 5c82ca3 into bitcoin:master Jul 20, 2022
@fanquake fanquake deleted the document_compat_code branch July 20, 2022 15:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 20, 2022
Merge bitcoin#25493: compat: document code in compat.h
224670e
@bitcoin bitcoin locked and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@luke-jr luke-jr luke-jr left review comments

@hebasto hebasto hebasto approved these changes

@vasild vasild vasild approved these changes

@laanwj laanwj Awaiting requested review from laanwj

@jamesob jamesob Awaiting requested review from jamesob

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@fanquake @jamesob @DrahtBot @vasild @laanwj @luke-jr @hebasto @maflcko