[Bug]: HRESULT_FROM_WIN32 in hresultcategory is a function and does not evaluate to a constant expression

[michaelKle]

bit7z version

4.0.x RC

Compilation options

No response

7-zip version

v22.01

7-zip shared library used

7z.dll / 7z.so

Compilers

MSVC

Compiler versions

MSVC 2019 with v141_xp toolkit

Architecture

x86

Operating system

Windows

Operating system versions

No response

Bug description

When compiling with VS 2019 and v141_xp toolkit the call HRESULT_FROM_WIN32 is not a macro but a function. This leads to a compilation error.

Steps to reproduce

No response

Expected behavior

Looking at WinError.h (where HRESULT_FROM_WIN32 is coming from) it seems the best option would be to simply use the provided macro __HRESULT_FROM_WIN32 or evaluate the expression before the switch statement in hresultcategory.cpp

Relevant compilation output

\build\_deps\bit7z-src\src\internal\hresultcategory.cpp(94): error C2131: expression did not evaluate to a constant
\build\_deps\bit7z-src\src\internal\hresultcategory.cpp(94): note: failure was caused by call of undefined function or one not declared 'constexpr'
\build\_deps\bit7z-src\src\internal\hresultcategory.cpp(94): note: see usage of 'HRESULT_FROM_WIN32'
\build\_deps\bit7z-src\src\internal\hresultcategory.cpp(94): error C2051: case expression not constant

Code of Conduct

• [X] By submitting this issue, I agree to follow bit7z's Code of Conduct

[rikyoz]

Hi!

When compiling with VS 2019 and v141_xp toolkit the call HRESULT_FROM_WIN32 is not a macro but a function. This leads to a compilation error.

Looking at WinError.h (where HRESULT_FROM_WIN32 is coming from) it seems the best option would be to simply use the provided macro __HRESULT_FROM_WIN32 or evaluate the expression before the switch statement in hresultcategory.cpp

Strangely, it did compile in the automatic builds! But yeah, I think that using the __HRESULT_FROM_WIN32 macro is the best option there. Anyway, thank you for pointing it out and for the pull request! 🙏

[michaelKle]

Thx for the library. We have been using our own version of a 7zip wrapper with Windows only compatibility and I was quite happy when I found your library.

[rikyoz]

My pleasure! I'm glad to hear that my library has been helpful. If you have any suggestions or feedback, feel free to let me know. Thanks!

[levicki]

@rikyoz

But yeah, I think that using the __HRESULT_FROM_WIN32 macro is the best option there.

The error text output by the compiler actually suggests that the proper solution would be:

constexpr HRESULT HRESULT_FROM_WIN32(const unsigned long x)
{
    return (HRESULT)(x) <= 0 ? (HRESULT)(x) : (HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000);
}

The only "challenge" would be naming it so it doesn't clash with Microsoft's own definition (you could use for example HR_FROM_WIN32).

The reason why Microsoft changed HRESULT_FROM_WIN32 from macro to an inline function was to avoid double evaluation of the macro parameter which can have unintended consequences.

[rikyoz]

The error text output by the compiler actually suggests that the proper solution would be:

constexpr HRESULT HRESULT_FROM_WIN32(const unsigned long x)
{
  return (HRESULT)(x) <= 0 ? (HRESULT)(x) : (HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000);
}

Yeah, I know. Usually, I too would prefer not to use a macro and instead use a constexpr function.

However, the __HRESULT_FROM_WIN32 macro is already provided by Microsoft, while the constexpr function is not. In this case, then, I preferred to use the more "native" solution on Windows.

On Linux and macOS, on the contrary, I decided to re-implement the HRESULT_FROM_WIN32 as a constexpr function, just like the one you suggested.

Nevertheless, I do not exclude that, in the future, I will use a single constexpr implementation on all platforms.

[levicki]

Nevertheless, I do not exclude that, in the future, I will use a single constexpr implementation on all platforms.

I already use my own constexpr implementations for a while, I am glad to see you are considering it too.