Conversation
- Use _BitScanForward fallback for 32-bit MSVC targets where _BitScanForward64 is unavailable - Define _WIN32_WINNT=0x0602 in mapper.cc before windows.h to ensure PrefetchVirtualMemory and WIN32_MEMORY_RANGE_ENTRY are available on MinGW Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| #ifndef _WIN32_WINNT | ||
| #define _WIN32_WINNT 0x0602 | ||
| #elif _WIN32_WINNT < 0x0602 | ||
| #undef _WIN32_WINNT |
There was a problem hiding this comment.
Just silently redefining this doesn't seem great. How about #error marisa-trie requires ...?
(Hi Carbo!)
| #if (defined _WIN32) || (defined _WIN64) | ||
| // Require Windows 8+ for PrefetchVirtualMemory / WIN32_MEMORY_RANGE_ENTRY | ||
| #ifndef _WIN32_WINNT | ||
| #define _WIN32_WINNT 0x0602 |
There was a problem hiding this comment.
There was a problem hiding this comment.
Latest run was 6 months ago and it does not seems had issues... xmake-io/xmake-repo#8215
MinGW itself has that defined here down below https://github.com/mirror/mingw-w64/blob/93f3505a758fe70e56678f00e753af3bc4f640bb/mingw-w64-headers/include/sdkddkver.h#L23
I just had issue with MinGW but not issue found for Windows, maybe we can promote if (MINGW) to if (WIN32) but I am not sure that is what PR tends to do.
https://cmake.org/cmake/help/v3.8/variable/WIN32.html
Can you try just changing to if(WIN32) and check if that works for windows MSVC x86 @BYVoid
There was a problem hiding this comment.
maybe we can promote
if (MINGW)toif (WIN32)
Yes, that sounds reasonable.
- mapper.cc: Replace silent _WIN32_WINNT redefinition with #error when the value is too low, instead of silently overriding it. - CMakeLists.txt: Set _WIN32_WINNT for all Windows targets (WIN32), not just MinGW, so MSVC also gets the correct definition. https://claude.ai/code/session_01L3jK8DJupby9nSzWredf95
|
after some trials, I believe we must provide an alternative to Windows environments that don't support Do you think if we can skip if (flags & MARISA_MAP_POPULATE) {
WIN32_MEMORY_RANGE_ENTRY range_entry;
range_entry.VirtualAddress = origin_;
range_entry.NumberOfBytes = size_;
::PrefetchVirtualMemory(GetCurrentProcess(), 1, &range_entry, 0);
}when |
Yes, that's the equivalent of skipping it when
I think this should just be an error. You should define what versions you want to support.
0x0602 is Windows 8. Windows 7 was EOL in 2020 or 2023 depending on how you want to count it. Why is this relevant at all? |
|
Can you make a PR for the bazel support? Then it could include a |
Instead of requiring _WIN32_WINNT >= 0x0602 (Windows 8) at compile time, conditionally compile the PrefetchVirtualMemory call so builds targeting older Windows versions simply skip the prefetch optimization. This also reverts CMakeLists.txt back to only setting _WIN32_WINNT for MinGW. https://claude.ai/code/session_01L3jK8DJupby9nSzWredf95
|
#143 is the Bazel PR. In this PR I will keep "Skip PrefetchVirtualMemory when _WIN32_WINNT < 0x0602", because this simple change would make Marisa Trie compatible with older systems, even though they are EOL. In addition, in some CI environments, |
| std::system_category(), "MapViewOfFile"); | ||
|
|
||
| if (flags & MARISA_MAP_POPULATE) { | ||
| #if defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x0602 |
There was a problem hiding this comment.
Nit: maybe move #if outside of if ().
- Move #if guard outside the if(flags) block per jmr's suggestion - Set _WIN32_WINNT for all Windows targets (WIN32) not just MinGW per luadebug's suggestion https://claude.ai/code/session_01L3jK8DJupby9nSzWredf95
4 participants
Summary
_BitScanForwardfallback inbit-vector.ccfor 32-bit MSVC targets where_BitScanForward64is unavailablePrefetchVirtualMemoryandWIN32_MEMORY_RANGE_ENTRYPorted from BYVoid/OpenCC#1078.
Test plan
🤖 Generated with Claude Code