Skip to content

[fix][Windows] MSVC compatibility for std::uniform_int_distribution with small types #301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zhiningli wants to merge 1 commit into from
Closed

[fix][Windows] MSVC compatibility for std::uniform_int_distribution with small types #301

zhiningli wants to merge 1 commit into from

Conversation

@zhiningli
Copy link
Contributor

@zhiningli zhiningli commented Dec 13, 2025

Summary

MSVC's std::uniform_int_distribution<T> requires T to be short or larger per C++ standard N4950 [rand.req.genl]/1.5. GCC/Clang are more permissive and allow uint8_t/int8_t.

This adds compat_uniform_distribution.h with cross-platform wrappers:

  • compat_uniform_int_distribution<T>
  • compat_binomial_distribution<T>
  • compat_geometric_distribution<T>

On MSVC, small types (uint8_t, int8_t) are promoted to short/unsigned short internally, then cast back. On GCC/Clang, the wrappers are simple type aliases to std:: types.

No performance impact - the promotion is a single cast at generation time, identical to what std::uniform_int_distribution<short> would produce.

Fixes #300

Note: Together with #299 (missing STL includes), this brings Windows test pass rate to 1208/1209 (99.9%). The remaining failure (ErrorsTest.BinaryTestArgTypesDeducedInC) is a platform-specific error message formatting difference and will be addressed in a future diff.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Test Plan

Built and ran full test suite on Windows:
ershell
cmake -S . -B build -G "Visual Studio 14 2015" -T ClangCL
cmake --build build --config Release
cd build && ctest -C Release --output-on-failure --timeout 60Result: 1208/1209 tests passing (99.9%)

  • 1 platform-specific error message formatting test (known MSVC difference, future fix)
image

Test Configuration

  • Compiler: clang-cl (ClangCL toolset)
  • Generator: Visual Studio 14 2015 with ClangCL toolset (-T ClangCL)
  • Build type: Release
  • Platform: Windows 11 x64

@zhiningli zhiningli self-assigned this Dec 13, 2025
@zhiningli zhiningli added bug Something isn't working build Build system and scripts labels Dec 13, 2025
@meta-cla meta-cla bot added the cla signed label Dec 13, 2025
@zhiningli zhiningli mentioned this pull request Dec 13, 2025
Closed
1 task
@zhiningli
Copy link
Contributor Author

zhiningli commented Dec 13, 2025

there is a tests\datagen\random_producer\uniform_distribution.h which does not seem to be used anywhere, maybe implementation can be integrated inside.

@zhiningli zhiningli mentioned this pull request Dec 13, 2025
Closed
@zhiningli zhiningli removed their assignment Dec 15, 2025
@meta-codesync
Copy link

meta-codesync bot commented Dec 19, 2025

@zhiningli has imported this pull request. If you are a Meta employee, you can view this in D89547407.

@zhiningli zhiningli force-pushed the windows-uniform-distribution branch from 4a809cd to 1b114fb Compare December 19, 2025 14:35
meta-codesync bot pushed a commit that referenced this pull request Dec 20, 2025
@zhiningli @meta-codesync
add missing STL includes (#299)
a9bfe98 
Summary:
Adds missing STL headers required for Windows/clang-cl builds:

- `<string>` for `std::to_string` in `Logger.h`
- `<numeric>` for `std::accumulate`, `std::iota` in test and training files
- `<stdexcept>` for `std::runtime_error` in `InputSet*.cpp`
- `<array>` for `std::array` in `test_functionGraphs.cpp`
- `<optional>` for `std::optional` in `InputSetBuilder.h`
- `<chrono>` for `std::chrono::high_resolution_clock` in trainer files
- Added MSVC detection (`_MSC_VER`) for IEEE 754 floating-point support in `portability.h`

These headers are implicitly included via transitive dependencies on GCC/Clang but MSVC requires explicit includes. **No runtime or performance impact** - purely compile-time fixes.

Fixes #95

**Note:** I have also validated that #97 (std::future move-only fix) works correctly on my Windows setup.

## Type of Change

- [x] Bug fix (non-breaking change which fixes an issue)

Pull Request resolved: #299

Test Plan:
Built the project

cmake -S . -B build -G "Visual Studio 14 2015" -T ClangCL
cmake --build build --config Release

build is now successful
The only error is for libzstd_shared.vcxproj (the shared/DLL version of zstd):
```fatal error RC1012: mismatched parenthesis : missing '```
This is a resource compiler issue with Clang headers when building the shared library. But this is non-blocking

After build successful, subsequent fix is about passing test, correcting ```std::future``` using PR from #97 and std::uniform distribution #301

### Test Configuration

- **Compiler:** clang-cl (ClangCL toolset)
- **Generator:** Visual Studio 14 2015 with ClangCL toolset (`-T ClangCL`)
- **Build type:** Release
- **Platform:** Windows 11 x64

Reviewed By: Cyan4973

Differential Revision: D89547335

Pulled By: zhiningli

fbshipit-source-id: 9fef41317cd73e9505c4fbef59b6041a5ad2a848
meta-codesync bot pushed a commit that referenced this pull request Dec 20, 2025
@zhiningli @meta-codesync
Enable error message type printing on clang-cl and fix cross-platform…
3924936 
… enum test (#302)

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:

- **Linux/GCC:** unsigned int
- **Windows/MSVC ABI:** int
- **ARM64**: May use either depending on 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:**

```
// Explicitly check for both possibilities using exact string matching
const char* _found_int  = strstr(_str, "(int)");
const char* _found_uint = strstr(_str, "(unsigned int)");
if (!_found_int && !_found_uint) {
    ZL_RET_R_ERR(GENERIC, "Message '(int)' or '(unsigned int)' not found...");
}
```

## Type of Change

- \[x\] Bug fix (non-breaking change which fixes an issue)

Pull Request resolved: #302

Test Plan:
**Test affected:** ErrorsTest.BinaryTestArgTypesDeducedInC (Test #306)

Built and ran the full test suite on Windows.
```
cmake --build build --config Release
cd build && ctest -C Release --timeout 60
```

**Result:**

**1209/1209 tests passing (100%)** ✅

<img width="891" height="359" alt="image" src="https://github.com/user-attachments/assets/e13fc529-c1fa-4676-879d-26d71786b182" />

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 |

Reviewed By: Cyan4973

Differential Revision: D89547438

Pulled By: zhiningli

fbshipit-source-id: 4e4b089a3265ff66669572d72cb7ff4555f1e5cb
@zhiningli @facebook-github-bot
MSVC compatibility for std::uniform_int_distribution with small types (…
4a9256b 
…facebook#301)

Summary:
MSVC's `std::uniform_int_distribution<T>` requires `T` to be `short` or larger per C++ standard N4950 [rand.req.genl]/1.5. GCC/Clang are more permissive and allow `uint8_t`/`int8_t`.

This adds `compat_uniform_distribution.h` with cross-platform wrappers:
- `compat_uniform_int_distribution<T>`
- `compat_binomial_distribution<T>`
- `compat_geometric_distribution<T>`

On MSVC, small types (`uint8_t`, `int8_t`) are promoted to `short`/`unsigned short` internally, then cast back. On GCC/Clang, the wrappers are simple type aliases to `std::` types.

**No performance impact** - the promotion is a single cast at generation time, identical to what `std::uniform_int_distribution<short>` would produce.

Fixes facebook#300

**Note:** Together with facebook#299 (missing STL includes), this brings Windows test pass rate to **1208/1209 (99.9%)**. The remaining failure (`ErrorsTest.BinaryTestArgTypesDeducedInC`) is a platform-specific error message formatting difference and will be addressed in a future diff.

## Type of Change

- [x] Bug fix (non-breaking change which fixes an issue)


Test Plan:
Built and ran full test suite on Windows:
ershell
cmake -S . -B build -G "Visual Studio 14 2015" -T ClangCL
cmake --build build --config Release
cd build && ctest -C Release --output-on-failure --timeout 60**Result:** 1208/1209 tests passing (99.9%)
- 1 platform-specific error message formatting test (known MSVC difference, future fix)
<img width="839" height="201" alt="image" src="https://github.com/user-attachments/assets/7aa15bc9-dac7-4f63-95a8-a9ec69b2dae8" />

### Test Configuration

- **Compiler:** clang-cl (ClangCL toolset)
- **Generator:** Visual Studio 14 2015 with ClangCL toolset (`-T ClangCL`)
- **Build type:** Release
- **Platform:** Windows 11 x64

Reviewed By: Cyan4973

Differential Revision: D89547407

Pulled By: zhiningli
@zhiningli zhiningli force-pushed the windows-uniform-distribution branch from a213dc9 to 4a9256b Compare December 20, 2025 13:51
@meta-codesync
Copy link

meta-codesync bot commented Dec 20, 2025

@zhiningli has exported this pull request. If you are a Meta employee, you can view the originating Diff in D89547407.

@meta-codesync meta-codesync bot closed this in f119c71 Dec 20, 2025
@meta-codesync
Copy link

meta-codesync bot commented Dec 20, 2025

@zhiningli merged this pull request in f119c71.

@zhiningli zhiningli self-assigned this Dec 20, 2025
@zhiningli zhiningli deleted the windows-uniform-distribution branch December 23, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot Awaiting requested review from facebook-github-bot

Assignees

@zhiningli zhiningli

Labels

bug Something isn't working build Build system and scripts cla signed fb-exported Merged meta-exported

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Windows] MSVC: std::uniform_int_distribution doesn't support uint8_t/int8_t

2 participants

@zhiningli @facebook-github-bot