Move Win32 defines to configure.ac to ensure they are globally defined #15704
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think this needs analogous MSVC build system changes. |
luke-jr
force-pushed
the
win32_defines_globally
branch
from
033ddef
to
e38c194
Compare
|
Fixed |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
maflcko
changed the title
maflcko
changed the title
|
tACK e38c194 with msvc x64. (haven't tested the mingw build) |
|
FWIW: I've verified that a disassembly of the |
|
@luke-jr Needs rebase |
luke-jr
force-pushed
the
win32_defines_globally
branch
from
e38c194
to
8c5535f
Compare
|
Rebased |
|
Concept ACK |
luke-jr
force-pushed
the
win32_defines_globally
branch
from
8c5535f
to
072a5e4
Compare
|
Rebased (macOS native Travis failing is "normal" now, right?) |
|
Open-Close to re-run ci. See #15847 (comment) |
Gitian builds
|
common.vcxproj used for MSVC builds
|
Rebased |
luke-jr
force-pushed
the
win32_defines_globally
branch
from
072a5e4
to
1ccb9f3
Compare
fanquake
approved these changes
Aug 25, 2020
There was a problem hiding this comment.
ACK 1ccb9f3 - checked that the binaries produced are the same.
If anyone wants to read into the origins/effects of these definitions, here are a couple articles:
Where did WIN32_LEAN_AND_MEAN come from?
What's the difference between WINVER, _WIN32_WINNT, _WIN32_WINDOWS, and _WIN32_IE?.
While reviewing I did check the time & lines saved in preprocessing by passing WIN32_LEAN_AND_MEAN. i.e:
#include <windows.h>
int main() {
return 0;
}# x86_64-w64-mingw32-g++ (GCC) 10-win32 20200525
# mingw-w64 7.0.0
time x86_64-w64-mingw32-g++ -E test.cpp | wc -l
80856 lines
avg time of 0.40s
time x86_64-w64-mingw32-g++ -E test.cpp -DWIN32_LEAN_AND_MEAN
54995 lines
avg time of 0.13s
# also checked with more of our defines
time x86_64-w64-mingw32-g++ -E test.cpp -D_WIN32_WINNT=0x0601 -D_WIN32_IE=0x0501 -DWIN32_LEAN_AND_MEAN -D_MT -DWIN32 -D_WINDOWS -D_FILE_OFFSET_BITS=64 | wc -l
56884 lines
avg time ~ the same as -DWIN32_LEAN_AND_MEANI might follow up with some documentation additions to configure.ac.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 25, 2020
…y are globally defined 1ccb9f3 Move Win32 defines to configure.ac to ensure they are globally defined (Luke Dashjr) Pull request description: bitcoin#9245 no longer needs this, since the main `_WIN32_WINNT` got bumped by something else. So rather than just lose it, might as well get it merged in independently. I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere. ACKs for top commit: fanquake: ACK 1ccb9f3 - checked that the binaries produced are the same. Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 2, 2020
Summary: ``` [...] I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere. ``` Backport of core [[bitcoin/bitcoin#15704 | PR15704]]. Depends on D7741. Test Plan: ``` cmake -GNinja .. \ -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win64.cmake \ -DBUILD_BITCOIN_SEEDER=OFF ninja ``` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7742
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#9245 no longer needs this, since the main
_WIN32_WINNTgot bumped by something else.So rather than just lose it, might as well get it merged in independently.
I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere.