Skip to content

compat: move (win) S_* defines into bdb #26832

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 1 commit into from
Apr 5, 2023
Merged

compat: move (win) S_* defines into bdb #26832

merged 1 commit into from
Apr 5, 2023

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jan 6, 2023 •
edited
Loading

This is the only place these defines are used. They may also be available when building for Windows. sys/stat.h is available, and we already use it unguarded in other code. So move the defines into bdb, after the stat.h include, and remove compat from bdb.cpp.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 6, 2023 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@hebasto
Copy link
Member

hebasto commented Jan 6, 2023

Concept ACK.

@fanquake fanquake force-pushed the dont_exclude_stat_win_bdb branch from 77d6f3c to cc16ab1 Compare February 14, 2023 15:29
@fanquake fanquake mentioned this pull request Feb 14, 2023
Closed
@fanquake
Copy link
Member Author

Rebased on #27098.

@fanquake fanquake marked this pull request as ready for review February 17, 2023 14:39
@fanquake fanquake force-pushed the dont_exclude_stat_win_bdb branch from cc16ab1 to cf0d86e Compare February 17, 2023 14:39
@TheCharlatan
Copy link
Contributor

I feel like I'm missing some context here. Why move these out of compat? There are also other symbols like WSAEAGAIN that are only used in one other file.

@DrahtBot DrahtBot mentioned this pull request Mar 15, 2023
Merged
@fanquake
Copy link
Member Author

I feel like I'm missing some context here. Why move these out of compat?

My plan was to split compat up further, so we didn't have the singular compat.h, with everything thrown in.

@TheCharlatan
Copy link
Contributor

Light code review ACK cf0d86e

@fanquake
refactor: don't avoid sys/types.h on when building for Windows
54e4061 
We've already used it unguarded in `httpserver.cpp` for years, with no
build issues.
@fanquake fanquake force-pushed the dont_exclude_stat_win_bdb branch from cf0d86e to 54e4061 Compare April 3, 2023 13:45
@fanquake
Copy link
Member Author

fanquake commented Apr 3, 2023

Rebased for #27254.

hebasto
hebasto approved these changes Apr 4, 2023
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 54e4061, I have reviewed the code and it looks OK.

nit: Two commits "refactor: don't avoid sys/types.h on when building for Windows" and "compat: move (win) S_* defines into bdb" were squashed but the commit message of the latter was lost.

@DrahtBot DrahtBot requested a review from TheCharlatan April 4, 2023 19:08
@TheCharlatan
Copy link
Contributor

re-ACK 54e4061

@DrahtBot DrahtBot removed the request for review from TheCharlatan April 4, 2023 20:45
@fanquake fanquake merged commit 23a899b into bitcoin:master Apr 5, 2023
@fanquake fanquake deleted the dont_exclude_stat_win_bdb branch April 5, 2023 10:36
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 5, 2023
@fanquake
Merge bitcoin#26832: compat: move (win) S_* defines into bdb
fd5d560 
54e4061 refactor: don't avoid sys/types.h on when building for Windows (fanquake)

Pull request description:

  This is the only place these defines are used. They may also be available when building for Windows. `sys/stat.h` is available, and we already use it unguarded in other code. So move the defines into bdb, after the stat.h include, and remove compat from bdb.cpp.

ACKs for top commit:
  TheCharlatan:
    re-ACK 54e4061
  hebasto:
    ACK 54e4061, I have reviewed the code and it looks OK.

Tree-SHA512: b75bb120654b4dec9ccc83aa407ee1a50969fec92f196a3722ec51282b91ac50e455af04f07211f3e93270ab83660f1efdeef43928b44b1e4296f6b06ea807c8
@bitcoin bitcoin locked and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fanquake @DrahtBot @hebasto @TheCharlatan