Skip to content

Disable benchmark for slide_hash_c with Visual C++ too.#2009

Merged
Dead2 merged 1 commit intozlib-ng:developfrom
mtl1979:vsbench
Nov 16, 2025
Merged

Disable benchmark for slide_hash_c with Visual C++ too.#2009
Dead2 merged 1 commit intozlib-ng:developfrom
mtl1979:vsbench

Conversation

@mtl1979
Copy link
Copy Markdown
Collaborator

@mtl1979 mtl1979 commented Nov 16, 2025

If WITH_ALL_FALLBACKS is unset, we need to disable benchmark for slide_hash_c when using Visual C++ to target AMD64.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 16, 2025

Walkthrough

Modified a preprocessor condition in the benchmark file to recognize the MSVC 64-bit architecture macro _M_X64 alongside the existing GCC/Clang __x86_64__ macro. The c variant benchmark is now excluded on both x86_64 and MSVC 64-bit builds unless the WITH_ALL_FALLBACKS flag is defined.

Changes

Cohort / File(s) Summary
Build configuration update
test/benchmarks/benchmark_slidehash.cc		 Updated preprocessor condition for BENCHMARK_SLIDEHASH(c, ...) from #if defined(WITH_ALL_FALLBACKS) || !defined(__x86_64__) to #if defined(WITH_ALL_FALLBACKS) || !(defined(__x86_64__) || defined(_M_X64)) to include MSVC 64-bit architecture detection

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately describes the main change: disabling the slide_hash_c benchmark on Visual C++ (MSVC) 64-bit builds, which is exactly what the code modification accomplishes.
Description check ✅ Passed The pull request description is directly related to the changeset, explaining the specific condition (WITH_ALL_FALLBACKS unset) and the purpose (disabling slide_hash_c benchmark with Visual C++ targeting AMD64).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c917ed6 and e20d034.

📒 Files selected for processing (1)
  • test/benchmarks/benchmark_slidehash.cc (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings 
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/x86_intrins.h:114-117
Timestamp: 2025-02-23T16:51:54.545Z
Learning: In x86/x86_intrins.h, the Clang macros for _mm_cvtsi64x_si128 and _mm_cvtsi128_si64x don't need additional MSVC guards since MSVC's implementation is already protected by `defined(_MSC_VER) && !defined(__clang__)`, making them mutually exclusive.

Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/x86_intrins.h:0-0
Timestamp: 2025-02-23T16:50:50.925Z
Learning: MSVC does not define `__GNUC__`, so adding `!defined(_MSC_VER)` to GCC detection macros is redundant when `__GNUC__` is already being checked.
📚 Learning: 2025-02-23T16:51:54.545Z 
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/x86_intrins.h:114-117
Timestamp: 2025-02-23T16:51:54.545Z
Learning: In x86/x86_intrins.h, the Clang macros for _mm_cvtsi64x_si128 and _mm_cvtsi128_si64x don't need additional MSVC guards since MSVC's implementation is already protected by `defined(_MSC_VER) && !defined(__clang__)`, making them mutually exclusive.

Applied to files:

  • test/benchmarks/benchmark_slidehash.cc
📚 Learning: 2025-06-18T19:28:32.987Z 
Learnt from: phprus
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/slide_hash_lsx.c:64-64
Timestamp: 2025-06-18T19:28:32.987Z
Learning: In zlib-ng's slide_hash functions, when calling slide_hash_chain(), the fourth parameter should be the window size (wsize) representing the number of entries in the prev table, not s->w_size directly. The local variable wsize is typically cast to the appropriate type (uint16_t) from s->w_size for this purpose.

Applied to files:

  • test/benchmarks/benchmark_slidehash.cc
📚 Learning: 2025-02-23T16:50:50.925Z 
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/x86_intrins.h:0-0
Timestamp: 2025-02-23T16:50:50.925Z
Learning: MSVC does not define `__GNUC__`, so adding `!defined(_MSC_VER)` to GCC detection macros is redundant when `__GNUC__` is already being checked.

Applied to files:

  • test/benchmarks/benchmark_slidehash.cc
🧬 Code graph analysis (1)
test/benchmarks/benchmark_slidehash.cc (1)
arch/x86/slide_hash_sse2.c (1)
  • void (54-63)
🔇 Additional comments (1)
test/benchmarks/benchmark_slidehash.cc (1)

80-82: LGTM - Correct MSVC x86-64 detection added.

The change correctly extends the architecture check to include MSVC's _M_X64 macro alongside GCC/Clang's __x86_64__. This appropriately excludes the generic C benchmark on MSVC x86-64 builds (where optimized SSE2/AVX2 implementations are available) unless WITH_ALL_FALLBACKS is explicitly set, aligning MSVC behavior with GCC/Clang.

Tip

📝 Customizable high-level summaries are now available!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide custom instructions to shape the summary (bullet lists, tables, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example:

"Create a concise high-level summary as a bullet-point list. Then include a Markdown table showing lines added and removed by each contributing author."

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 16, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.55%. Comparing base (c917ed6) to head (e20d034).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2009      +/-   ##
===========================================
+ Coverage    80.05%   81.55%   +1.50%     
===========================================
  Files          162      162              
  Lines        13558    12813     -745     
  Branches      3474     3156     -318     
===========================================
- Hits         10854    10450     -404     
+ Misses        1640     1293     -347     
- Partials      1064     1070       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Dead2
Dead2 approved these changes Nov 16, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@Dead2 Dead2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dead2 Dead2 merged commit 228ed7b into zlib-ng:develop Nov 16, 2025
155 of 156 checks passed
@Dead2 Dead2 mentioned this pull request Nov 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dead2 Dead2 Dead2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mtl1979 @Dead2