yhirose/cpp-httplib

'#if WINAPI_FAMILY_PARTITION' usage

yhirose opened this issue · 15 comments

yhirose commented

@mol123, @sergio-nsk could you explain why there are 4 different #if cases which look pretty much similar to each other. Is it possible to use just one of them for consistency and code readability? Thanks!

#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8)
#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)
#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8)
#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8)
mol123 commented

The reason is that the Windows SDK headers defines the respective functions conditionally on exactly these (complicated) expressions.

You can have have a look into the following header files:

  • fileapi.h for CreateFile2, GetFileSizeEx
  • memoryapi.h for CreateFileMappingFromApp

So, some of the functions might or might not be available based on which of the features are enabled and based on which Windows version is getting targeted.

My understanding is, that #1773 introduced the code in the if branches to support UWP apps. The else branches are the code for targeting older Windows versions and for the case, when the "new" functions are not available on the targeted configuration.

yhirose commented

@mol123 thanks for the explanation. 👍
@sergio-nsk could you give me your comment?

sergio-nsk commented 
  1. #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8)

    This condition seems to be copied form fileapi.h of the Windows SDK. It looks well.

  2. #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)

    IMO it's not needed, GetFileSizeEx() is available on XP and Server 2003 and can be used unconditionally. If LONGLONG LARGE_INTEGER::QuadPart is an issue, you need to upgrade Windows XP SDK or easy solve it with if constexpr (sizeof(size_t) == 8)).

  3. #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8)

    This condition seems to be copied form memoryapi.h of the Windows SDK. It looks well.

  4. #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8)

    This condition seems to be copied form memoryapi.h of the Windows SDK. It looks well.

So, IMO 2 is not needed.

Windows SDK is changeable, Game partition is persistently being extended. I would just use #if _WIN32_WINNT >= _WIN32_WINNT_WIN8 in 1, 3, 4.

sergio-nsk commented

BTW WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES) means all Windows families, is always true and can be removed.

yhirose commented

@mol123 could you replay the comments from @sergio-nsk, since all these complicated conditions have been introduced at c8bcaf8 that you contributed? Thanks!

mol123 commented
  • I can move 1, 3, 4 to #if _WIN32_WINNT >= _WIN32_WINNT_WIN8
  • I can move 2 to use GetFileSizeEx unconditionally.

There still remains the question, what to do with 32 bit systems (having a 32 bit size_t). I would add some conditional code of _WIN64 supporting these, just to avoid getting issues filed if somebody tries to compile for a 32 bit system. @yhirose, what do you think?

sergio-nsk commented

There is no any issues with compiling for 32-bit Windows if the Windows Kit is actual.

mol123 commented

But even with an up-to date Windows Kit, sizeof(size_t) will be 4 when targeting 32-bit Windows, so we won't be able to store the 64-bit value returned by GetFileSizeEx in size_.

sergio-nsk commented 
if (!::GetFileSizeEx(hFile_, &size) || size.QuadPart > std::numeric_limits<decltype(size_)>::max()) { return false; }

But no clue why you are warring about that, 32 Windows can't map files > 2GB.

sergio-nsk commented 
  hMapping_ = ::CreateFileMappingW(hFile_, NULL, PAGE_READONLY, size.HighPart,
                                   size.LowPart, NULL);

should be

  hMapping_ = ::CreateFileMappingW(hFile_, NULL, PAGE_READONLY, 0, 0, NULL);

If dwMaximumSizeLow and dwMaximumSizeHigh are 0 (zero), the maximum size of the file mapping object is equal to the current size of the file that hFile identifies.

mol123 commented

Created #1909 with proposal for code changes.

yhirose commented

I have just merged the pull request. Thanks!

antimo-pv commented

To compile the new version I had to change line 2875
from
std::numeric_limits<size_t>::max())
to
(std::numeric_limits<size_t>::max)())
Unfortunately max(a, b) is a macro in Windows.h, which I can't dispose of because it's used in many other parts of my code. Maybe this can be useful to others experiencing the same problem.

antimo-pv commented

Sorry, from
std::numeric_limits<decltype(size_)>::max())
to
(std::numeric_limits<decltype(size_)>::max)())
I made a copy&paste mistake...

  • 👍1
yhirose commented

@antimo-pv thanks for the report. I fixed it.