Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Jan 6, 2023

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

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
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
@sedited
Copy link
Contributor

sedited commented Feb 20, 2023

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.

@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.

@sedited
Copy link
Contributor

sedited commented Mar 23, 2023

Light code review ACK cf0d86ed15041822e2a0a55ddf231af7809a9e66

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.

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 sedited April 4, 2023 19:08
@sedited
Copy link
Contributor

sedited commented Apr 4, 2023

re-ACK 54e4061

@DrahtBot DrahtBot removed the request for review from sedited 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
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants