[fix][Windows] Enable error message type printing on clang-cl and fix cross-platform enum test #302
Conversation
This file contains hidden or 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
|
@zhiningli has imported this pull request. If you are a Meta employee, you can view this in D89547438. |
…tform enum test - Enable ZL_ENABLE_RET_IF_ARG_PRINTING for clang-cl by adding __clang__ check - Fix enum type test to work on both Linux (unsigned int) and Windows (int) The error message type printing feature was disabled on clang-cl because the preprocessor check only looked for __GNUC__. Since clang-cl defines __clang__, adding this check enables the feature. The enum type test expected '(unsigned int)' but Windows/MSVC ABI uses '(int)' for C enums. Changed to match '(int' which works on both platforms.
The substring '(int' was too permissive and could match unintended types like '(int32_t)'. Changed to explicitly check for both '(int)' and '(unsigned int)' using exact string matching, which works across all platforms (Linux, Windows, ARM64).
zhiningli
force-pushed
the
windows-error-type-printing
branch
from
7822e25 to
92dea58
Compare
|
@zhiningli merged this pull request in 3924936. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Enables the error message type printing feature (ZL_ENABLE_RET_IF_ARG_PRINTING) on clang-cl and fixes a cross-platform test incompatibility.
Issue 1: Type printing disabled on clang-cl
File: include/openzl/detail/zl_errors_detail.h (line 570)
The preprocessor check for enabling verbose error messages only checked for _GNUC_. However, clang-cl defines _clang_ instead, causing the feature to remain disabled on Windows/Clang builds.
Before:
if defined(\__GNUC_\_) && !defined(\__cplusplus)After:
if (defined(\__GNUC_\_) || defined(\__clang_\_)) && !defined(\__cplusplus)Issue 2: Enum type differs between platforms
File: tests/unittest/common/test_errors_in_c.c (line 223)
C enums have different underlying types depending on the platform ABI:
The test previously expected a strict string match that failed on Windows.
Before:
EXPECT_ERROR_MESSAGE_CONTAINS(e_1, e_2, "(unsigned int)");After:
Type of Change
Test Plan
Test affected: ErrorsTest.BinaryTestArgTypesDeducedInC (Test #306)
Built and ran the full test suite on Windows.
Result:
1209/1209 tests passing (100%) ✅
After windows compatibility fix #97, #299, #301
Specifically verified the affected test:
ctest -C Release -R "ErrorsTest.BinaryTestArgTypesDeducedInC" --output-on-failure
Output:
# Result: 1/1 Test #306: ErrorsTest.BinaryTestArgTypesDeducedInC ... Passed
Test Configuration
| Component | Detail |
| Compiler | clang-cl (ClangCL toolset) |
| Generator | Visual Studio with ClangCL toolset (-T ClangCL) |
| Build type | Release |
| Platform | Windows 11 x64 |