ci: Test on Windows #46
Conversation
WalkthroughThis update introduces a comprehensive Windows CI/CD setup for building and testing a PHP extension. It adds PowerShell scripts for installing dependencies, building the extension, and running tests. A new GitHub Actions workflow orchestrates these scripts across a matrix of PHP and Visual Studio versions. Minor source and configuration updates improve compiler compatibility, code portability, and runtime CPU feature detection. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Runner (Windows)
participant install.ps1
participant build.ps1
participant test.ps1
GitHub Actions->>Runner (Windows): Start job (matrix config)
Runner (Windows)->>install.ps1: Run install.ps1
install.ps1->>Runner (Windows): Setup PHP SDK, binaries, deps, APCu
Runner (Windows)->>build.ps1: Run build.ps1
build.ps1->>Runner (Windows): Build PHP extension, copy DLL, configure php.ini
Runner (Windows)->>test.ps1: Run test.ps1
test.ps1->>Runner (Windows): Run PHP tests, report result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
JakubOnderka
force-pushed
the
build-windows
branch
3 times, most recently
from
285527a to
b120cfd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (8)
.github/workflows/test.ps1 (1)
5-9: Consider adding output capture for better debugging.While the current error handling is good, consider capturing the test output to a variable or log file for better debugging when tests fail in CI.
-$env:TEST_PHP_EXECUTABLE = "C:\php\bin\php.exe" -& $env:TEST_PHP_EXECUTABLE 'run-tests.php' --color --show-diff tests -if (-not $?) { - throw "testing failed with errorlevel $LastExitCode" -} +$env:TEST_PHP_EXECUTABLE = "C:\php\bin\php.exe" +$output = & $env:TEST_PHP_EXECUTABLE 'run-tests.php' --color --show-diff tests 2>&1 +if (-not $?) { + Write-Host "Test output: $output" + throw "testing failed with errorlevel $LastExitCode" +}.github/workflows/install.ps1 (4)
44-44: Fix typo in error message.There's a typo in the download error message.
- echo "Downlaod: https://windows.php.net/downloads/releases/archives/$bname" + echo "Download: https://windows.php.net/downloads/releases/archives/$bname"
85-86: Clarify the APCu version exclusion logic.The condition excludes PHP 8.2 and 8.3 from APCu installation, but the reasoning isn't clear. Consider adding a comment explaining why these versions are excluded.
-# PECL apuc -if (('' -ne $env:PECL_APCU) -and ('8.2' -ne $env:PHP_VER) -and ('8.3' -ne $env:PHP_VER)) { +# PECL APCu - excluded for PHP 8.2 and 8.3 due to compatibility issues +if (('' -ne $env:PECL_APCU) -and ('8.2' -ne $env:PHP_VER) -and ('8.3' -ne $env:PHP_VER)) {
87-87: Consider parameterizing the APCu version.The APCu version is hardcoded. Consider making it configurable through an environment variable for easier updates.
- $apcu_version = '5.1.21' + $apcu_version = if ($env:APCU_VERSION) { $env:APCU_VERSION } else { '5.1.21' }
110-124: Consider using a loop for APCu header file moves.The repetitive
Move-Itemcommands for APCu header files could be simplified using a loop for better maintainability.- Move-Item "C:\php\apcu-$apcu_version\php_apc.h" 'C:\php\devel\include\ext\apcu\php_apc.h' - Move-Item "C:\php\apcu-$apcu_version\apc.h" 'C:\php\devel\include\ext\apcu\apc.h' - Move-Item "C:\php\apcu-$apcu_version\apc_api.h" 'C:\php\devel\include\ext\apcu\apc_api.h' - Move-Item "C:\php\apcu-$apcu_version\apc_cache.h" 'C:\php\devel\include\ext\apcu\apc_cache.h' - Move-Item "C:\php\apcu-$apcu_version\apc_globals.h" 'C:\php\devel\include\ext\apcu\apc_globals.h' - Move-Item "C:\php\apcu-$apcu_version\apc_iterator.h" 'C:\php\devel\include\ext\apcu\apc_iterator.h' - Move-Item "C:\php\apcu-$apcu_version\apc_lock.h" 'C:\php\devel\include\ext\apcu\apc_lock.h' - Move-Item "C:\php\apcu-$apcu_version\apc_mutex.h" 'C:\php\devel\include\ext\apcu\apc_mutex.h' - Move-Item "C:\php\apcu-$apcu_version\apc_sma.h" 'C:\php\devel\include\ext\apcu\apc_sma.h' - Move-Item "C:\php\apcu-$apcu_version\apc_serializer.h" 'C:\php\devel\include\ext\apcu\apc_serializer.h' - Move-Item "C:\php\apcu-$apcu_version\apc_stack.h" 'C:\php\devel\include\ext\apcu\apc_stack.h' - Move-Item "C:\php\apcu-$apcu_version\apc_windows_srwlock_kernel.h" 'C:\php\devel\include\ext\apcu\apc_windows_srwlock_kernel.h' - Move-Item "C:\php\apcu-$apcu_version\apc_arginfo.h" 'C:\php\devel\include\ext\apcu\apc_arginfo.h' - Move-Item "C:\php\apcu-$apcu_version\php_apc_legacy_arginfo.h" 'C:\php\devel\include\ext\apcu\php_apc_legacy_arginfo.h' + $apcu_headers = @( + 'php_apc.h', 'apc.h', 'apc_api.h', 'apc_cache.h', 'apc_globals.h', + 'apc_iterator.h', 'apc_lock.h', 'apc_mutex.h', 'apc_sma.h', + 'apc_serializer.h', 'apc_stack.h', 'apc_windows_srwlock_kernel.h', + 'apc_arginfo.h', 'php_apc_legacy_arginfo.h' + ) + foreach ($header in $apcu_headers) { + Move-Item "C:\php\apcu-$apcu_version\$header" "C:\php\devel\include\ext\apcu\$header" + }.github/workflows/windows.yml (3)
3-7: Add pull_request trigger and branch filters.Currently, CI runs on every push and on published releases. To validate incoming PRs and avoid running on all branch pushes, consider:
- Adding a
pull_request:trigger (e.g., branches:[main]).- Restricting
push:to specific branches (main,release/*, etc.).
58-62: Ensure tags are fetched in checkout.You're triggering on
release, but the default checkout is shallow (no tags). Addfetch-depth: 0:- uses: actions/checkout@v4 with: persist-credentials: false fetch-depth: 0
71-82: Use consistent path separators and consider a setup action.Prefer forward slashes or quoted backslashes to avoid YAML escape issues:
-run: .\.github\workflows\install.ps1 +run: ./.github/workflows/install.ps1Also, you could simplify maintenance by using an established action (e.g.
shivammathur/setup-php) instead of custom install scripts.
🛑 Comments failed to post (4)
.github/workflows/test.ps1 (1)
3-3: 💡 Verification agent
🧩 Analysis chain
Verify PATH directories match the installation script.
The hardcoded PATH directories should align with the structure created by the
install.ps1script. Ensure consistency across the workflow scripts.
🏁 Script executed:
#!/bin/bash # Description: Check PATH consistency between test.ps1 and install.ps1 # Expected: The directories in PATH should match those created by install.ps1 # Check what directories are referenced in the PATH in test.ps1 rg "env:PATH.*=.*" .github/workflows/test.ps1 # Check what directories are created in install.ps1 rg "C:\\\\php\\\\(devel|bin|deps)" .github/workflows/install.ps1Length of output: 2160
Align PATH entries in test.ps1 with install.ps1
The
test.ps1workflow currently prependsC:\php\deps\bin, but the install script only createsC:\php\deps. Update the PATH entry for consistency:• File: .github/workflows/test.ps1
Line 3- $env:PATH = "C:\php\devel;C:\php\bin;C:\php\deps\bin;$env:PATH" + $env:PATH = "C:\php\devel;C:\php\bin;C:\php\deps;$env:PATH"🤖 Prompt for AI Agents
In .github/workflows/test.ps1 at line 3, the PATH environment variable includes "C:\php\deps\bin" which does not match the directory structure created by install.ps1 that only creates "C:\php\deps". Update the PATH entry by replacing "C:\php\deps\bin" with "C:\php\deps" to ensure consistency between the test and install scripts..github/workflows/build.ps1 (1)
5-13: 💡 Verification agent
🧩 Analysis chain
Investigate the build failure and improve debugging.
The pipeline shows "building failed with errorlevel 2". Consider adding more verbose output to help diagnose build issues.
-$task = New-Item 'task.bat' -Force -Add-Content $task 'call phpize 2>&1' -Add-Content $task "call configure --with-php-build=C:\php\deps --enable-$env:PHP_EXT --enable-debug-pack 2>&1" -Add-Content $task 'nmake /nologo 2>&1' -Add-Content $task 'exit %errorlevel%' -& "C:\php\php-sdk-$env:BIN_SDK_VER\phpsdk-$env:VS-$env:ARCH.bat" -t $task -if (-not $?) { - throw "building failed with errorlevel $LastExitCode" -} +$task = New-Item 'task.bat' -Force +Add-Content $task '@echo off' +Add-Content $task 'echo Running phpize...' +Add-Content $task 'call phpize 2>&1' +Add-Content $task 'if errorlevel 1 (echo phpize failed with errorlevel %errorlevel% & exit /b %errorlevel%)' +Add-Content $task 'echo Running configure...' +Add-Content $task "call configure --with-php-build=C:\php\deps --enable-$env:PHP_EXT --enable-debug-pack 2>&1" +Add-Content $task 'if errorlevel 1 (echo configure failed with errorlevel %errorlevel% & exit /b %errorlevel%)' +Add-Content $task 'echo Running nmake...' +Add-Content $task 'nmake /nologo 2>&1' +Add-Content $task 'exit %errorlevel%' +Write-Host "Executing build with: C:\php\php-sdk-$env:BIN_SDK_VER\phpsdk-$env:VS-$env:ARCH.bat" +& "C:\php\php-sdk-$env:BIN_SDK_VER\phpsdk-$env:VS-$env:ARCH.bat" -t $task +if (-not $?) { + Write-Host "Build task.bat contents:" + Get-Content $task + throw "building failed with errorlevel $LastExitCode" +}
🏁 Script executed:
#!/bin/bash # Description: Check for potential build configuration issues # Expected: Identify common causes of build failures # Check if configure script expects specific parameters rg "configure.*enable.*simdjson" --type bat --type cmd # Look for build configuration files that might affect the build fd "config.w32|config.m4" -x cat {} # Check for any build dependencies mentioned in the codebase rg "(dependency|require|need)" config.w32Length of output: 2620
Improve Windows CI build script with detailed logging and error handling
To diagnose the “errorlevel 2” failure more effectively, enhance
.github/workflows/build.ps1so that each step intask.batis echoed, failures are caught immediately, and the full task contents are printed on error. This will surface whether phpize, configure or nmake is the culprit.• File:
.github/workflows/build.ps1
• Section: lines 5–13 (task.bat generation and execution)-$task = New-Item 'task.bat' -Force -Add-Content $task 'call phpize 2>&1' -Add-Content $task "call configure --with-php-build=C:\php\deps --enable-$env:PHP_EXT --enable-debug-pack 2>&1" -Add-Content $task 'nmake /nologo 2>&1' -Add-Content $task 'exit %errorlevel%' -& "C:\php\php-sdk-$env:BIN_SDK_VER\phpsdk-$env:VS-$env:ARCH.bat" -t $task -if (-not $?) { - throw "building failed with errorlevel $LastExitCode" -} +$task = New-Item 'task.bat' -Force +Add-Content $task '@echo off' +Add-Content $task 'echo === Running phpize ===' +Add-Content $task 'call phpize 2>&1' +Add-Content $task 'if errorlevel 1 (echo phpize failed with exit code %errorlevel% & exit /b %errorlevel%)' + +Add-Content $task 'echo === Running configure ===' +Add-Content $task "call configure --with-php-build=C:\php\deps --enable-$env:PHP_EXT --enable-debug-pack 2>&1" +Add-Content $task 'if errorlevel 1 (echo configure failed with exit code %errorlevel% & exit /b %errorlevel%)' + +Add-Content $task 'echo === Running nmake ===' +Add-Content $task 'nmake /nologo 2>&1' +Add-Content $task 'exit %errorlevel%' + +Write-Host "Invoking PHP SDK initialization script:" +Write-Host " C:\php\php-sdk-$env:BIN_SDK_VER\phpsdk-$env:VS-$env:ARCH.bat -t $task" +& "C:\php\php-sdk-$env:BIN_SDK_VER\phpsdk-$env:VS-$env:ARCH.bat" -t $task +if (-not $?) { + Write-Host "=== task.bat contents on failure ===" + Get-Content $task | Write-Host + throw "Build failed with exit code $LastExitCode" +}These changes will immediately pinpoint which phase fails and preserve the full log for review.
🧰 Tools
🪛 GitHub Actions: Windows
[error] 12-12: Build script threw exception: building failed with errorlevel 2.
🤖 Prompt for AI Agents
In .github/workflows/build.ps1 around lines 5 to 13, enhance the task.bat script by adding commands to echo each step before execution to provide verbose output. Modify the script to immediately exit on any failure within task.bat to catch errors early. After running the task, if it fails, output the full contents of task.bat and the captured logs to help identify whether phpize, configure, or nmake caused the error. This will improve debugging by making the build process and failure points more transparent..github/workflows/windows.yml (2)
8-12:
⚠️ Potential issueGuard
PHP_EXT_VERSIONfor non-release events.
PHP_EXT_VERSIONusesgithub.event.release.tag_name, which is undefined on plain pushes. Either:
- Scope it under a
release–only job.- Provide a fallback (e.g.
${{ github.ref_name }}or empty string).🤖 Prompt for AI Agents
In .github/workflows/windows.yml around lines 8 to 12, the environment variable PHP_EXT_VERSION is set to github.event.release.tag_name, which is undefined for non-release events like plain pushes. To fix this, modify the workflow to either run this job only on release events or provide a fallback value for PHP_EXT_VERSION using an expression like github.ref_name or an empty string to avoid errors during non-release triggers.
48-48: 🛠️ Refactor suggestion
Use a ternary for
runs-on.GitHub expressions support
condition ? trueValue : falseValuebut not JS-style&&/||for value selection. Update to:runs-on: ${{ matrix.vs == 'vs17' ? 'windows-2022' : 'windows-2019' }}🤖 Prompt for AI Agents
In .github/workflows/windows.yml at line 48, the runs-on expression incorrectly uses JavaScript-style && and || operators for conditional value selection, which GitHub Actions does not support. Replace the expression with a ternary operator using the syntax: matrix.vs == 'vs17' ? 'windows-2022' : 'windows-2019' to correctly select the runner based on the matrix.vs value.
JakubOnderka
force-pushed
the
build-windows
branch
from
b120cfd to
324d1b4
Compare
JakubOnderka
force-pushed
the
build-windows
branch
from
324d1b4 to
4c9dab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/windows.yml (1)
17-22: Consider enabling additional PHP versions or updating the matrix excludes.Currently only PHP 8.3 is enabled, but the matrix excludes (lines 30-39) reference commented-out versions, causing actionlint warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/build.ps1(1 hunks).github/workflows/install.ps1(1 hunks).github/workflows/test.ps1(1 hunks).github/workflows/windows.yml(1 hunks)config.w32(1 hunks)src/simdjson_avx2.h(1 hunks)src/simdjson_vector8.h(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/simdjson_avx2.h
- .github/workflows/test.ps1
🚧 Files skipped from review as they are similar to previous changes (3)
- config.w32
- .github/workflows/install.ps1
- .github/workflows/build.ps1
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/windows.yml
30-30: value "8.4" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
34-34: value "8.2" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
36-36: value "8.1" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
38-38: value "8.0" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
🔇 Additional comments (3)
src/simdjson_vector8.h (2)
26-36: LGTM! Excellent cross-platform compatibility improvement.The
_trailing_zeroesfunction properly abstracts the platform-specific implementations for counting trailing zeros, enabling Windows MSVC support while maintaining compatibility with other compilers.
73-75: Clean migration to the abstracted function.The replacement of direct
__builtin_ctzllcalls with_trailing_zeroesmaintains the same functionality while providing cross-platform compatibility..github/workflows/windows.yml (1)
41-76: Well-structured Windows CI workflow.The workflow configuration is comprehensive with appropriate caching, environment setup, and build steps for Windows development with Visual Studio.
| - php: '8.4' | ||
| vs: vs16 | ||
| - php: '8.3' | ||
| vs: vs17 | ||
| - php: '8.2' | ||
| vs: vs17 | ||
| - php: '8.1' | ||
| vs: vs17 | ||
| - php: '8.0' | ||
| vs: vs17 |
There was a problem hiding this comment.
Remove matrix excludes for non-existent PHP versions.
The exclude entries reference PHP versions (8.4, 8.2, 8.1, 8.0) that are not currently included in the matrix, causing static analysis warnings.
Either uncomment the PHP versions in the matrix or remove these exclude entries:
- exclude:
- - php: '8.4'
- vs: vs16
- - php: '8.3'
- vs: vs17
- - php: '8.2'
- vs: vs17
- - php: '8.1'
- vs: vs17
- - php: '8.0'
- vs: vs17
+ exclude:
+ - php: '8.3'
+ vs: vs17📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - php: '8.4' | |
| vs: vs16 | |
| - php: '8.3' | |
| vs: vs17 | |
| - php: '8.2' | |
| vs: vs17 | |
| - php: '8.1' | |
| vs: vs17 | |
| - php: '8.0' | |
| vs: vs17 | |
| exclude: | |
| - php: '8.3' | |
| vs: vs17 |
🧰 Tools
🪛 actionlint (1.7.7)
30-30: value "8.4" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
34-34: value "8.2" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
36-36: value "8.1" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
38-38: value "8.0" in "exclude" does not match in matrix "php" combinations. possible values are "8.3"
(matrix)
🤖 Prompt for AI Agents
In .github/workflows/windows.yml around lines 30 to 39, the matrix excludes
reference PHP versions 8.4, 8.2, 8.1, and 8.0 which are not present in the
matrix, causing static analysis warnings. To fix this, either uncomment and
include these PHP versions in the matrix or remove the corresponding exclude
entries that mention these versions to keep the matrix consistent and avoid
warnings.
d39a1d0 to
5517235
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/windows.yml (1)
29-39: Remove matrix excludes for non-existent PHP versionsExclude entries for PHP 8.3–8.0 don’t match the
phpmatrix and will cause actionlint warnings. Please remove these entries:- - php: '8.3' - vs: vs17 - - php: '8.2' - vs: vs17 - - php: '8.1' - vs: vs17 - - php: '8.0' - vs: vs17🧰 Tools
🪛 actionlint (1.7.7)
32-32: value "8.3" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
34-34: value "8.2" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
36-36: value "8.1" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
38-38: value "8.0" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
🧹 Nitpick comments (3)
.github/workflows/windows.yml (3)
3-7: Specify branch filters for push triggersCurrently the
pushevent triggers on all branches, which may lead to unnecessary workflow runs. Consider restricting it to main or release branches:on: push: branches: - main - release/* release: types: [published]
17-22: Clean up commented-out PHP versionsThe commented matrix entries for PHP 8.3–8.0 add noise. If these versions aren’t planned imminently, remove these lines; otherwise, un-comment and include them in the matrix:
- # - '8.3' - # - '8.2' - # - '8.1' - # - '8.0'
63-69: Prefer forward slashes in script pathsUsing forward slashes improves readability and consistency across platforms:
- - name: Install build command - run: .\.github\workflows\install.ps1 + - name: Install build command + run: ./.github/workflows/install.ps1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/windows.yml(1 hunks)src/simdjson_avx2.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/simdjson_avx2.h
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/windows.yml
32-32: value "8.3" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
34-34: value "8.2" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
36-36: value "8.1" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
38-38: value "8.0" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
🔇 Additional comments (2)
.github/workflows/windows.yml (2)
29-32: Verify the PHP 8.4 + vs16 exclusion is intentionalBy excluding the
php: '8.4'+vs: vs16combination, all VS2019 builds for PHP 8.4 will be skipped, leaving only VS2022. Confirm this is deliberate, or remove the exclusion to include VS16 coverage.🧰 Tools
🪛 actionlint (1.7.7)
32-32: value "8.3" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
1-74: Overall workflow setup looks solidThe matrix strategy, caching, and step sequence for checkout/install/build/test are well-structured.
🧰 Tools
🪛 actionlint (1.7.7)
32-32: value "8.3" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
34-34: value "8.2" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
36-36: value "8.1" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
38-38: value "8.0" in "exclude" does not match in matrix "php" combinations. possible values are "8.4"
(matrix)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simdjson_encoder.cpp(1 hunks)src/simdjson_vector8.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/simdjson_vector8.h
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci (8.4, nts, vs17)
- GitHub Check: ci (8.4, ts, vs17)
🔇 Additional comments (1)
src/simdjson_encoder.cpp (1)
533-542:❌ Incorrect review comment
Verify CPUID intrinsics availability and add missing headers.
The fallback implementation uses
__cpuidand__cpuidexintrinsics, but these require appropriate headers and are compiler-specific. The logic itself is correct for detecting AVX2 support.Run the following script to check if the necessary headers are included:
The CPUID detection logic is correct, but ensure that:
#include <intrin.h>is included for MSVC- Alternative implementations exist for GCC/Clang if needed
- The function is properly tested across different compilers
🏁 Script executed:
#!/bin/bash # Description: Check for CPUID intrinsic headers and usage patterns in the codebase # Expected: Find include statements for intrin.h, immintrin.h, or cpuid.h echo "Checking for CPUID-related headers:" rg -i "include.*(?:intrin|cpuid|immintrin)\.h" --type cpp echo -e "\nChecking for other CPUID usage patterns:" rg "__cpuid|__cpuidex|_cpuid" --type cpp -A 2 -B 2Length of output: 15091
🏁 Script executed:
#!/bin/bash # Inspect header includes in src/simdjson_encoder.cpp to verify CPUID intrinsic headers echo "Header includes in src/simdjson_encoder.cpp:" rg -n "^#include" src/simdjson_encoder.cppLength of output: 694
🏁 Script executed:
#!/bin/bash # Check for CPUID fallback macros and intrinsics in compatibility header echo "Searching for GCC CPUID macros in simdjson_compatibility.h:" rg -n "HAVE_GCC_GET_CPUID|USE_GCC_GET_CPUID" src/simdjson_compatibility.h echo -e "\nSearching for __cpuid definitions in simdjson_compatibility.h:" rg -n "__cpuid" src/simdjson_compatibility.hLength of output: 390
No Missing CPUID Headers – Already Included Transitively
The AVX2 detection in
src/simdjson_encoder.cppis covered by existing includes:
simdjson_avx2.hbrings in<intrin.h>(MSVC) and<x86intrin.h>(GCC/Clang).simdjson.halso includes<cpuid.h>.- GCC/Clang support for
__cpuid/__cpuidexis provided via<x86intrin.h>.No additional headers or alternate fallbacks are needed; the current intrinsics logic and testing across compilers remains valid as-is.
Likely an incorrect or invalid review comment.
| #ifdef __builtin_cpu_supports | ||
| return __builtin_cpu_supports("avx2"); // check support in runtime | ||
| #endif |
There was a problem hiding this comment.
Fix the conditional compilation check for __builtin_cpu_supports.
The current check #ifdef __builtin_cpu_supports will not work as expected because __builtin_cpu_supports is a GCC builtin function, not a preprocessor macro. This condition will likely always be false.
Consider using a compiler-specific check instead:
-#ifdef __builtin_cpu_supports
+#if defined(__GNUC__) && !defined(__clang__) || (defined(__clang__) && __has_builtin(__builtin_cpu_supports))
return __builtin_cpu_supports("avx2"); // check support in runtime
#endifOr use feature detection:
-#ifdef __builtin_cpu_supports
+#if __has_builtin(__builtin_cpu_supports)
return __builtin_cpu_supports("avx2"); // check support in runtime
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifdef __builtin_cpu_supports | |
| return __builtin_cpu_supports("avx2"); // check support in runtime | |
| #endif | |
| #if defined(__GNUC__) && !defined(__clang__) || (defined(__clang__) && __has_builtin(__builtin_cpu_supports)) | |
| return __builtin_cpu_supports("avx2"); // check support in runtime | |
| #endif |
🤖 Prompt for AI Agents
In src/simdjson_encoder.cpp around lines 529 to 531, the conditional compilation
check using #ifdef __builtin_cpu_supports is incorrect because
__builtin_cpu_supports is a GCC builtin function, not a macro. Replace this
check with a compiler-specific macro such as #if defined(__GNUC__) &&
!defined(__clang__) to ensure the code compiles only when using GCC, or use
feature detection macros like __AVX2__ to conditionally compile the AVX2 support
check. Adjust the preprocessor directives accordingly to correctly detect
compiler support before calling __builtin_cpu_supports.
Summary by CodeRabbit
New Features
Chores
Bug Fixes