Skip to content

[fix][Windows][clang-cl build] add missing STL includes #299

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 2 commits into from
Closed

[fix][Windows][clang-cl build] add missing STL includes #299

zhiningli wants to merge 2 commits into from

Conversation

@zhiningli
Copy link
Contributor

@zhiningli zhiningli commented Dec 13, 2025 •
edited
Loading

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

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

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

@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
This was referenced Dec 13, 2025
Closed
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 D89547335.

@zhiningli
fix: add missing STL includes for Windows/clang-cl build
436c5f1 
Adds missing headers required for Windows builds:
- <string> for std::to_string
- <numeric> for std::accumulate, std::iota
- <stdexcept> for std::runtime_error
- <array> for std::array
- <optional> for std::optional
- <chrono> for std::chrono::high_resolution_clock
- MSVC detection for IEEE 754 in portability.h

Fixes facebook#95
@zhiningli
style: apply clang-format
ac7d30e
@zhiningli zhiningli force-pushed the windows-missing-includes branch from c05e237 to ac7d30e Compare December 19, 2025 14:37
@meta-codesync meta-codesync bot closed this in a9bfe98 Dec 20, 2025
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
@meta-codesync
Copy link

meta-codesync bot commented Dec 20, 2025

@zhiningli merged this pull request in a9bfe98.

zhiningli added a commit to zhiningli/openzl that referenced this pull request Dec 20, 2025
@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
meta-codesync bot pushed a commit that referenced this pull request Dec 20, 2025
@zhiningli @meta-codesync
MSVC compatibility for std::uniform_int_distribution with small types (…
f119c71 
…#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 #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

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

Pull Request resolved: #301

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

fbshipit-source-id: 18a0b3ae02a2006d5d54b4f221db543dbba94f87
@zhiningli zhiningli self-assigned this Dec 20, 2025
@zhiningli zhiningli deleted the windows-missing-includes 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 Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows/clang-cl tools fail to build — missing includes + copying move-only std::future

2 participants

@zhiningli @facebook-github-bot