fix: replace C++20 designated initializers with C++17 compatible assignments - #448
Conversation
…gnments Add strict CMake flags to avoid MSVC errors during builds
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe root CMakeLists.txt is renamed to ChangesStrict C++ Flags Rollout
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
github-actions
Bot
added
example-app
|
@coderabbitai which line in your config is making you skip reviewing this PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain⚙️ CodeRabbit configuration inspected Length of output: 17763 🏁 Script executed: #!/bin/bash
gh pr view 448 --repo Ryan-Millard/Img2Num --json isDraft,title,state,baseRefName,labels,authorLength of output: 830 🏁 Script executed: #!/bin/bash
echo "=== PR Comments ==="
gh pr view 448 --repo Ryan-Millard/Img2Num --json comments --jq '.comments[] | {author: .author.login, body: .body[:200]}'
echo ""
echo "=== PR Files Changed ==="
gh pr view 448 --repo Ryan-Millard/Img2Num --json files --jq '.files[].path'Length of output: 974 🏁 Script executed: #!/bin/bash
gh pr view 448 --repo Ryan-Millard/Img2Num --json comments --jq '.comments[0].body'Length of output: 3827
reviews:
auto_review:
base_branches: []An empty To fix it, add reviews:
auto_review:
base_branches:
- devYou can also use regex patterns, e.g. |
|
@Krasner please take note of these changes. I didn't want to make them but we want to publish code that works everywhere and is usable. This failure is part of what caused the Python library to fail when publishing. Instead of upgrading the version, I chose to stay on C++17 because it would allow more compatibility and less future refactoring. It's also a very stable version. |
Sounds good to me! |
|
@coderabbitai I want you to review every PR regardless of the branch. I need proper reviews from you all the time so we can catch bugs before it's too late. Please tell me what I need to update to get you to do that here. # .coderabbit.yaml ΓÇö CodeRabbit configuration for Img2Num
# Docs: https://docs.coderabbit.ai/
language: en-US
reviews:
profile: assertive # Img2Num has a C/C++ core ΓÇö stricter feedback is warranted
request_changes_workflow: true # Block merge until CodeRabbit comments are resolved
review_details: true
high_level_summary: true
high_level_summary_instructions: |
Create concise release notes as a bullet-point list,
followed by a Markdown table showing lines added and removed by each contributing author.
Remember to frequently update the high-level summary when code changes.
high_level_summary_in_walkthrough: true
collapse_walkthrough: false
poem: true
slop_detection:
enabled: true
label: "slop" # Applied when a PR looks like AI-generated spam
auto_apply_labels: true
auto_review:
enabled: true
auto_incremental_review: true
drafts: false
ignore_title_keywords:
- "WIP"
- "wip"
- "[draft]"
# Exclude vendored / generated / lockfile paths from review
path_filters:
- "!third_party/**"
- "!**/node_modules/**"
- "!**/dist/**"
- "!**/build/**"
- "!pnpm-lock.yaml"
- "!**/versioned_docs/**" # auto-generated Docusaurus versioned copies
path_instructions:
- path: "core/**/*.{cpp,c,h,hpp}"
instructions: |
This is the Img2Num core C/C++ library. Review for:
- Memory safety: null pointer dereferences, use-after-free, buffer overflows.
- Correct RAII usage and smart pointer idioms.
- Adherence to the .clang-format style; formatting must be applied via
`./img2num format-cpp` (or `./img2num format-wasm` for WASM modules),
NOT by calling clang-format directly.
- When suggesting build/test steps, always use the Docker-first wrapper scripts
(`./img2num`, `img2num.ps1`, or `img2num.bat`) rather than direct tool invocations,
since contributors may not have dependencies installed locally.
- Must have Doxygen docstrings in the API (`img2num.h`) files. This is important!
- path: "**/*.{cpp,c,h,hpp}"
instructions: |
- For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via `./img2num format-cpp`.
- Doxygen docstrings are required.
- path: "docs/docs/**"
instructions: |
All documentation must follow Docusaurus conventions (docs/docs/ folder structure).
Do NOT suggest creating strangely-named markdown files at the repository root
(e.g. COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, SUMMARY.md).
New docs belong either as a dedicated Docusaurus category or integrated into an
existing category such as project-scripts. Verify valid frontmatter
(title, sidebar_label, etc.) and correct sidebar wiring.
- path: "docs/**/*.{js,jsx,ts,tsx,mdx}"
instructions: |
This is the Docusaurus site source. Check for MDX compatibility, correct
plugin usage, and valid imports. Review docusaurus.config.js changes carefully
for broken routing or plugin config regressions.
- path: ".github/workflows/**"
instructions: |
GitHub Actions workflows. Review for:
- SHA-pinned action versions for third-party actions (security best practice).
- Secrets accessed only via ${{ secrets.* }} ΓÇö never hardcoded.
- Least-privilege permissions on each job/workflow.
- Correct job dependency ordering (needs:) and if/condition logic.
- path: "scripts/**"
instructions: |
These are the Img2Num developer scripts. When referencing CLI commands in review
comments, always prefer the top-level `./img2num` (shell), `img2num.ps1` (PowerShell),
or `img2num.bat` (CMD) wrapper over direct tool invocations, to support contributors
using Docker.
- path: "**/*.sh"
instructions: |
Shell scripts in this repository. Flag ShellCheck issues and POSIX compatibility
problems. When suggesting how to execute scripts, prefer the `./img2num` wrapper.
- path: "img2num.ps1"
instructions: |
PowerShell wrapper script for the img2num CLI. Ensure PSScriptAnalyzer compliance
and that all Docker commands align with the shell/batch equivalents.
- path: "img2num.bat"
instructions: |
Windows batch wrapper for the img2num CLI. Check for common batch scripting pitfalls
(quoting, delayed expansion, exit code propagation).
- path: "Dockerfile*"
instructions: |
Review with Hadolint rules. Prefer multi-stage builds, minimal base images,
and pinned image tags. Ensure no secrets are baked into layers.
- path: "bindings/**"
instructions: |
Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
- path: "example-apps/**"
instructions: |
- Example applications. These are for demonstration; keep them minimal and ensure
they correctly reflect the public API. Flag any use of internal/private APIs.
- **IMPORTANT:** These example apps must be a good reflection of how to use Img2Num's
library, which means that they need good comments and must have clean code. This
is meant to be external and is designed for others to use to get started.
- path: "**/*.wgsl"
instructions: |
WebGPU Shading Language (WGSL) shader files. Review for:
# Correctness & safety
- Correct use of address spaces: function, private, workgroup, uniform,
storage (read / read_write). Flag mismatched access modes on var<storage>.
- All entry points must declare a stage attribute (`@vertex`, `@fragment`,
or `@compute`). Flag missing or incorrect stage attributes.
- Recursion is forbidden in WGSL ΓÇö flag any call graph cycle.
- `@binding` and `@group` annotations must be present and non-overlapping
for all module-scope resource variables.
- Alignment/layout attributes (`@align`, `@size`, `@stride`) must match the
corresponding CPU-side buffer struct layout.
- Atomic operations (atomicLoad, atomicStore, atomicAdd, etc.) are only
valid on atomic<i32> / atomic<u32> in the workgroup or storage
address spaces ΓÇö flag incorrect usage.
# Performance
- Prefer workgroup-local accumulation over repeated storage reads in
compute shaders.
- Flag unnecessary use of `var` (mutable) where `let` (immutable) or
`const` (compile-time constant) would suffice.
- Flag `@workgroup_size` values that are not a multiple of the GPU's
subgroup size (typically 32 or 64) unless there is a documented reason.
# Portability
- The `f16` type requires `enable f16;` ΓÇö flag uses without the enable
directive and note that f16 availability is implementation-dependent.
- Flag use of unofficial/vendor-specific extensions that are not part of
the W3C WGSL spec.
# Style
- snake_case for variable and function names.
- Group `@binding` declarations together, sorted by `@group` then `@binding`.
- path: "**/*.py"
instructions: |
Python source files. Review for:
- PEP 8 compliance (enforced by ruff; focus on logic, not style).
- Type annotation coverage ΓÇö prefer typed signatures for public functions.
- Exception handling: avoid bare `except:` clauses; always catch specific exceptions.
- Resource management: use `with` statements for file/socket/connection handling.
- No mutable default arguments (e.g. `def foo(x=[]):`).
- When suggesting how to run Python tooling, prefer the `./img2num` Docker wrapper
rather than direct `python` / `ruff` / `pylint` invocations.
pre_merge_checks:
title:
mode: error
requirements: >
- The title must be specific and descriptive. Follow Conventional Commit prefixes
where applicable: feat:, fix:, docs:, chore:, refactor:, perf:, test:.
- Vague titles such as "Update files", "Fix bug", "Chore", or "Basic maintenance"
are NOT acceptable ΓÇö they must describe the actual change. We have a workflow
that handles this, but you should assist with edge-cases where it doesn't.
- Reference the [Conventional Commits Gist](https://gist.github.com/Zekfad/f51cb06ac76e2457f11c80ed705c95a3)
if a PR does not conform to the requirements explained there.
description:
mode: error # PRs with empty or boilerplate-only descriptions must not merge
issue_assessment:
mode: warning
custom_checks:
- name: No AI Slop PR Description
mode: error
instructions: |
Fail this check if the PR description contains generic AI-generated boilerplate
that does not describe the specific changes in this PR. Examples of failing text:
- "This contribution was made as part of an open-source documentation improvement initiative."
- "All changes are meant to be helpful additions to the project."
- Any templated language that could apply to any PR without modification.
The description must specifically explain WHAT was changed and WHY, for this PR.
Additionally, be wary of Markdown-only pull requests - they are often signs of slop - but
do not just blindly flag PRs that are Markdown-only.
- name: No Strangely-Named Root Markdown Files
mode: error
instructions: |
Fail if the PR adds any unconventional markdown files directly at the repository
root. Only these files are acceptable at root level: README.md, CONTRIBUTING.md,
CODE_OF_CONDUCT.md, CHANGELOG.md, SECURITY.md, LICENSE (no extension).
Files such as COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, SUMMARY.md,
CHANGES_OVERVIEW.md, or similar should NEVER be created at the root - they belong
in the Docusaurus documentation and a link to them may be placed in the README.md
if it is absolutely relevant.
- name: CodeRabbit Config Needs Update
mode: warning
instructions: |
Warn if this PR introduces any of the following WITHOUT a corresponding
update to `.coderabbit.yaml`:
1. **New programming languages** ΓÇö e.g., the first `.py`, `.rs`, `.go`,
`.java`, `.rb`, `.kt`, `.swift`, or similar source file appears in the
diff and there is no matching entry under `reviews.path_instructions`,
`tools:`, or `code_generation.docstrings.path_instructions` for that
language.
2. **New linter / formatter config files** ΓÇö e.g., `.flake8`, `pylintrc`,
`.pylintrc`, `pyproject.toml` (with `[tool.ruff]` / `[tool.pylint]`),
`.eslintrc*`, `.stylelintrc*`, `biome.json`, `.rubocop.yml`, etc. appear
without the corresponding tool being enabled or disabled under `tools:`.
3. **New file extensions not covered by existing path_instructions** ΓÇö e.g.,
`.wgsl`, `.proto`, `.rego`, `.tf`, `.prisma`, or any other extension
that currently has no matching glob in `reviews.path_instructions`.
4. **New CI / tooling configuration files** ΓÇö e.g., a new `.github/workflows/`
file that uses a third-party Action not yet reviewed in the workflows
path_instructions, or a new `dependabot.yml` section for a package
ecosystem not previously present.
**Pass** this check if `.coderabbit.yaml` is itself modified in this PR in
a way that covers the newly introduced language/tool/extension, OR if none
of the above conditions are triggered by the diff.
**Warn** (do not hard-fail) so the maintainer can consciously decide whether
an update is needed.
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
simplify:
enabled: true # Useful for the C++ core
tools:
# ΓöÇΓöÇ Relevant to Img2Num's stack ΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇ
clang:
enabled: true # C/C++ static analysis (core, bindings, wasm)
cppcheck:
enabled: true # C/C++ bug detection
shellcheck:
enabled: true # img2num shell script, .devcontainer/entrypoint.sh
psscriptanalyzer:
enabled: true # img2num.ps1 PowerShell wrapper
blinter:
enabled: true # img2num.bat Windows batch wrapper
hadolint:
enabled: true # Dockerfile.dev
eslint:
enabled: true # bindings/js, packages/js, docs JS/TS
actionlint:
enabled: true # .github/workflows/
yamllint:
enabled: true # .github/workflows/, dependabot.yml, labeler.yml
markdownlint:
enabled: true # docs, CONTRIBUTING.md, README.md, etc.
gitleaks:
enabled: true # Secret scanning
trufflehog:
enabled: true # Secret scanning (with verification)
osvScanner:
enabled: true # Dependency vulnerability scanning (pnpm, CMake)
opengrep:
enabled: true # Security pattern matching across all languages
semgrep:
enabled: true # Security/code-quality rules
dotenvLint:
enabled: true # docs/.env.example
htmlhint:
enabled: true # example-apps/react-js HTML
github-checks:
enabled: true
timeout_ms: 120000 # Give CI checks up to 2 min before review proceeds
ast-grep:
essential_rules: true # General AST pattern rules
# ΓöÇΓöÇ Disable tools not applicable to this repo ΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇΓöÇ
biome:
enabled: false # Project uses ESLint + Prettier, not Biome
ruff:
enabled: true
flake8:
enabled: false
pylint:
enabled: true
swiftlint:
enabled: false # No Swift
phpstan:
enabled: false # No PHP
phpmd:
enabled: false
phpcs:
enabled: false
golangci-lint:
enabled: false # No Go
rubocop:
enabled: false # No Ruby
brakeman:
enabled: false
detekt:
enabled: false # No Kotlin
buf:
enabled: false # No Protobuf
regal:
enabled: false # No Rego/OPA
pmd:
enabled: false # No Java
clippy:
enabled: false # No Rust
sqlfluff:
enabled: false # No SQL
tflint:
enabled: false # No Terraform
checkov:
enabled: false # No IaC (beyond Dockerfile, covered by Hadolint)
trivy:
enabled: false # No IaC
prismaLint:
enabled: false # No Prisma
circleci:
enabled: false # Using GitHub Actions, not CircleCI
luacheck:
enabled: false # No Lua
shopifyThemeCheck:
enabled: false
smartyLint:
enabled: false
emberTemplateLint:
enabled: false
fortitudeLint:
enabled: false # No Fortran
knowledge_base:
web_search:
enabled: true
code_guidelines:
enabled: true
# Point at CONTRIBUTING.md so CodeRabbit learns your contribution standards
filePatterns:
- "CONTRIBUTING.md"
- ".clang-format"
- ".editorconfig"
- "docs/docs/contributing/**"
learnings:
scope: local # Keep learnings scoped to this repo (it's public)
issues:
scope: local
pull_requests:
scope: local
code_generation:
docstrings:
language: en-US
path_instructions:
- path: "core/**/*.{cpp,c,h,hpp}"
instructions: |
Generate Doxygen-compatible docstrings using the /** ... */ block style.
Follow the conventions set in Doxyfile.public for public API headers
and Doxyfile.internal for internal implementation. Use `@param`, `@return`,
`@brief`, `@note` tags consistently.
- path: "bindings/js/**/*.{js,ts}"
instructions: |
Generate JSDoc-compatible docstrings using /** ... */ style with
`@param`, `@returns`, and `@throws` tags.
unit_tests:
path_instructions:
- path: "core/**"
instructions: |
When generating C++ unit tests, use the testing framework already present
in the project. Always suggest running tests via `./img2num` Docker wrapper
commands rather than direct invocations. |
|
auto_review:
enabled: true
auto_incremental_review: true
drafts: false
ignore_title_keywords:
- "WIP"
- "wip"
- "[draft]"to this: auto_review:
enabled: true
auto_incremental_review: true
drafts: false
base_branches:
- ".*" # Review PRs targeting ANY branch
ignore_title_keywords:
- "WIP"
- "wip"
- "[draft]"The In the meantime, I'll kick off a review of this PR right now. (´• ω •`) 🐇 ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/py/CMakeLists.txt`:
- Around line 16-18: The file(GLOB ...) command used to define BINDING_SRC does
not include the CONFIGURE_DEPENDS option, which means CMake will not
automatically reconfigure when new C++ source files are added to the src
directory. Add the CONFIGURE_DEPENDS option to the file(GLOB BINDING_SRC ...)
call to make it reconfiguration-safe and ensure newly added binding sources are
properly detected without requiring manual CMake reconfiguration.
In `@CMakeLists.txt`:
- Around line 7-8: Update the CMAKE_CXX_STANDARD value in CMakeLists.txt from 17
to 20 to align with the C++20 standard specified in the .clang-format
configuration file. Additionally, review and update the comment on line 9 that
explicitly states C++17 intent to reflect the new standard, and verify that the
-Werror=c++20-extensions flag on line 15 is still appropriate for C++20
enforcement or remove it if no longer needed.
- Around line 14-16: Fix the indentation for the IMG2NUM_STRICT_CXX_FLAGS set
command by changing from 4-space to 2-space indentation on lines 15 and 16. Add
the SHELL: prefix before the space-separated compiler flags in the GNU/Clang
generator expression (the flags like -Wpedantic, -Werror=pedantic,
-Werror=c++20-extensions) to ensure CMake properly interprets them during
expansion. Break line 15 into multiple lines to keep each line under the
120-character maximum. Additionally, locate CMAKE_CXX_STANDARD on line 7 and
change its value from 17 to 20 to match the repository's C++20 requirement.
In `@example-apps/console-c/CMakeLists.txt`:
- Line 28: The target_link_libraries call for CImg2NumExample_console_c
unconditionally links the math library `m`, which is not available on Windows
with MSVC toolchain and causes linker errors. Guard the linking of the `m`
library by wrapping it in a CMake conditional that checks if the compiler is not
MSVC (using if(NOT MSVC) or similar platform detection), so that `m` is only
linked on non-MSVC toolchains where the math library is actually available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33716a9b-b7ac-4834-9049-d0baea89d96b
📒 Files selected for processing (8)
CMakeLists.txtbindings/c/CMakeLists.txtbindings/c/src/cimg2num.cppbindings/py/CMakeLists.txtcore/CMakeLists.txtexample-apps/console-c/CMakeLists.txtexample-apps/console-cpp/CMakeLists.txtpyproject.toml
💤 Files with no reviewable changes (1)
- pyproject.toml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.txt
📄 CodeRabbit inference engine (.editorconfig)
**/*.txt: Do not enforce maximum line length for text files
Use 2-space indentation for text files
Files:
bindings/c/CMakeLists.txtexample-apps/console-c/CMakeLists.txtCMakeLists.txtbindings/py/CMakeLists.txtexample-apps/console-cpp/CMakeLists.txtcore/CMakeLists.txt
bindings/**
⚙️ CodeRabbit configuration file
bindings/**: Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
Files:
bindings/c/CMakeLists.txtbindings/py/CMakeLists.txtbindings/c/src/cimg2num.cpp
**
⚙️ CodeRabbit configuration file
**: # Contributing to Img2NumWant to contribute to Img2Num? There are a few things you need to know.
We wrote a contribution guide to help you get started.
A few important points:
- Add tests with your PR — new features and bug fixes must include tests where appropriate. PRs without tests are unlikely to be approved.
- Follow the repository's coding style rules.
- Use the issue and PR templates when filing issues or submitting code. Your PR will be rejected if you don't.
If you're unsure what to change, open a discussion and someone will assist you.
Questions?
If you have questions or need help:
- Open a discussion
- Create an issue
- Check existing PRs for ideas
Thank you for improving Img2Num! 🎨🚀
**: BasedOnStyle: LLVM
Standard: c++20--- Basic formatting ---
IndentWidth: 4
ColumnLimit: 100
TabWidth: 4--- Braces ---
Cpp11BracedListStyle: true
SpaceBeforeCpp11BracedList: true
BreakBeforeBraces: Attach--- Braced initializers ---
Cpp11BracedListStyle: true
--- Constructor initializer lists ---
PackConstructorInitializers: Never
BreakConstructorInitializers: BeforeComma
ConstructorInitializerIndentWidth: 4--- Alignment ---
AlignAfterOpenBracket: BlockIndent
--- Pointers ---
PointerAlignment: Left
--- Includes ---
IncludeBlocks: Regroup
SortIncludes: CaseInsensitive--- Extern "C" cleanliness ---
IndentExternBlock: NoIndent
--- Lambdas ---
AllowShortLambdasOnASingleLine: Inline
--- Functions ---
AllowShortFunctionsOnASingleLine: None
**: root = true-------------------------
Global defaults
-------------------------
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
max_line_lengt...
Files:
bindings/c/CMakeLists.txtexample-apps/console-c/CMakeLists.txtCMakeLists.txtbindings/py/CMakeLists.txtbindings/c/src/cimg2num.cppexample-apps/console-cpp/CMakeLists.txtcore/CMakeLists.txt
example-apps/**
⚙️ CodeRabbit configuration file
example-apps/**: - Example applications. These are for demonstration; keep them minimal and ensure
they correctly reflect the public API. Flag any use of internal/private APIs.
- IMPORTANT: These example apps must be a good reflection of how to use Img2Num's
library, which means that they need good comments and must have clean code. This
is meant to be external and is designed for others to use to get started.
Files:
example-apps/console-c/CMakeLists.txtexample-apps/console-cpp/CMakeLists.txt
{CMakeLists.txt,**/*.cmake}
📄 CodeRabbit inference engine (.editorconfig)
{CMakeLists.txt,**/*.cmake}: Use 2-space indentation for CMake configuration files
Maintain 120 character maximum line length for CMake files
Files:
CMakeLists.txt
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
bindings/c/src/cimg2num.cpp
**/*.{cpp,cc,cxx,c++,h,hpp,hxx,h++}
📄 CodeRabbit inference engine (.clang-format)
**/*.{cpp,cc,cxx,c++,h,hpp,hxx,h++}: Use LLVM coding style as the base style for C++ code
Use C++20 standard for all C++ code
Use an indent width of 4 spaces for C++ code
Limit line length to 100 columns in C++ code
Use 4 spaces for tab width in C++ code
Use C++11 braced list style with space before braced lists in C++ code
Attach opening braces to the same line (BreakBeforeBraces: Attach) in C++ code
Never pack constructor initializer lists on a single line in C++ code
Break constructor initializer lists before comma in C++ code
Use 4 spaces for constructor initializer list indentation in C++ code
Use block indent alignment after opening brackets in C++ code
Align pointers to the left in C++ code
Regroup and organize include blocks in C++ code
Sort includes case-insensitively in C++ code
Do not indent extern "C" blocks in C++ code
Allow short lambdas only inline on a single line in C++ code
Do not allow short functions on a single line in C++ code
Files:
bindings/c/src/cimg2num.cpp
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
bindings/c/src/cimg2num.cpp
🧠 Learnings (6)
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-04-27T15:40:33.329Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num C bindings (e.g., `bindings/c/src/cimg2num.cpp`) that expose `extern "C"` functions, do not allow C++ exceptions (including `std::bad_alloc` and exceptions thrown from lambdas used internally) to propagate out of these C binding functions. Use the C-API failure idiom instead: catch exceptions internally as needed, clear/record error state via the existing error mechanism, and return `nullptr` (or the documented sentinel return value) on failure so the C caller can detect errors by checking the return value.
Applied to files:
bindings/c/src/cimg2num.cpp
🔇 Additional comments (5)
bindings/c/src/cimg2num.cpp (2)
11-23: LGTM!
26-39: LGTM!core/CMakeLists.txt (1)
41-42: LGTM!bindings/c/CMakeLists.txt (1)
21-22: LGTM!example-apps/console-cpp/CMakeLists.txt (1)
4-25: LGTM!
| file(GLOB BINDING_SRC | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make source glob reconfiguration-safe.
Line 16 uses file(GLOB ...) without CONFIGURE_DEPENDS, so newly added binding sources can be skipped until a manual CMake reconfigure.
Suggested fix
-file(GLOB BINDING_SRC
- "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp"
-)
+file(GLOB BINDING_SRC CONFIGURE_DEPENDS
+ "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp"
+)📝 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.
| file(GLOB BINDING_SRC | |
| "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" | |
| ) | |
| file(GLOB BINDING_SRC CONFIGURE_DEPENDS | |
| "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/py/CMakeLists.txt` around lines 16 - 18, The file(GLOB ...) command
used to define BINDING_SRC does not include the CONFIGURE_DEPENDS option, which
means CMake will not automatically reconfigure when new C++ source files are
added to the src directory. Add the CONFIGURE_DEPENDS option to the file(GLOB
BINDING_SRC ...) call to make it reconfiguration-safe and ensure newly added
binding sources are properly detected without requiring manual CMake
reconfiguration.
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the CMakeLists.txt file to see lines 7-8 in context
head -20 CMakeLists.txt | cat -nRepository: Ryan-Millard/Img2Num
Length of output: 1101
🏁 Script executed:
# Search for C++ standard specifications in all configuration files
rg "CMAKE_CXX_STANDARD|Standard:" --type cmake --type confRepository: Ryan-Millard/Img2Num
Length of output: 93
🏁 Script executed:
# Check if .clang-format file exists and verify C++20 requirement
if [ -f .clang-format ]; then
echo "=== .clang-format content (relevant lines) ==="
cat .clang-format | grep -A 2 -B 2 "Standard"
fiRepository: Ryan-Millard/Img2Num
Length of output: 174
Resolve the C++17 vs C++20 standard mismatch.
CMakeLists.txt line 7 enforces C++17 (set(CMAKE_CXX_STANDARD 17)), but the repository coding guidelines (.clang-format) require C++20 (Standard: c++20). This creates a configuration conflict that must be resolved—either update CMakeLists.txt to enforce C++20, or explicitly update the project standards documentation and all configuration files to C++17.
Note: Line 9 contains a comment explicitly stating C++17 intent, and line 15 includes -Werror=c++20-extensions, suggesting this may be a deliberate choice. However, it contradicts the stated coding standards and requires alignment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CMakeLists.txt` around lines 7 - 8, Update the CMAKE_CXX_STANDARD value in
CMakeLists.txt from 17 to 20 to align with the C++20 standard specified in the
.clang-format configuration file. Additionally, review and update the comment on
line 9 that explicitly states C++17 intent to reflect the new standard, and
verify that the -Werror=c++20-extensions flag on line 15 is still appropriate
for C++20 enforcement or remove it if no longer needed.
Source: Coding guidelines
| set(IMG2NUM_STRICT_CXX_FLAGS | ||
| $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>,$<CXX_COMPILER_ID:AppleClang>>>:-Wpedantic -Werror=pedantic -Werror=c++20-extensions> | ||
| $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:MSVC>>:/permissive-> |
There was a problem hiding this comment.
Fix indentation, line length, and generator expression syntax for strict compiler flags.
Line 15 exceeds the 120-character maximum (174 chars) and both lines 15–16 use 4-space indentation instead of the required 2-space. The space-separated compiler flags in the GNU/Clang generator expression must be quoted with the SHELL: prefix to prevent CMake from misinterpreting them during expansion.
Suggested fix
set(IMG2NUM_STRICT_CXX_FLAGS
- $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>,$<CXX_COMPILER_ID:AppleClang>>>:-Wpedantic -Werror=pedantic -Werror=c++20-extensions>
- $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:MSVC>>:/permissive->
+ "$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>,$<CXX_COMPILER_ID:AppleClang>>>:SHELL:-Wpedantic -Werror=pedantic -Werror=c++20-extensions>"
+ "$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:MSVC>>:/permissive->"
)Additionally, line 7 sets CMAKE_CXX_STANDARD 17 but the repository requires C++20 per coding guidelines.
📝 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.
| set(IMG2NUM_STRICT_CXX_FLAGS | |
| $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>,$<CXX_COMPILER_ID:AppleClang>>>:-Wpedantic -Werror=pedantic -Werror=c++20-extensions> | |
| $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:MSVC>>:/permissive-> | |
| set(IMG2NUM_STRICT_CXX_FLAGS | |
| "$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>,$<CXX_COMPILER_ID:AppleClang>>>:SHELL:-Wpedantic -Werror=pedantic -Werror=c++20-extensions>" | |
| "$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:MSVC>>:/permissive->" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CMakeLists.txt` around lines 14 - 16, Fix the indentation for the
IMG2NUM_STRICT_CXX_FLAGS set command by changing from 4-space to 2-space
indentation on lines 15 and 16. Add the SHELL: prefix before the space-separated
compiler flags in the GNU/Clang generator expression (the flags like -Wpedantic,
-Werror=pedantic, -Werror=c++20-extensions) to ensure CMake properly interprets
them during expansion. Break line 15 into multiple lines to keep each line under
the 120-character maximum. Additionally, locate CMAKE_CXX_STANDARD on line 7 and
change its value from 17 to 20 to match the repository's C++20 requirement.
|
|
||
| # Link against the C bindings | ||
| target_link_libraries(console_c_app PRIVATE CImg2Num m) | ||
| target_link_libraries(CImg2NumExample_console_c PRIVATE CImg2Num m) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd example-apps/console-c && cat -n CMakeLists.txtRepository: Ryan-Millard/Img2Num
Length of output: 1126
🏁 Script executed:
# Check other example apps for patterns of conditional linking
find example-apps -name "CMakeLists.txt" -type f | head -10 | xargs cat -nRepository: Ryan-Millard/Img2Num
Length of output: 2173
🏁 Script executed:
# Search for existing conditional linking patterns in the main CMakeLists files
rg "if.*MSVC" --type cmakeRepository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
# Search for target_link_libraries with math library
rg "target_link_libraries.*\bm\b" --type cmake -A 2 -B 2Repository: Ryan-Millard/Img2Num
Length of output: 277
🌐 Web query:
MSVC libm math library Windows CMake linking
💡 Result:
When developing with the MSVC compiler on Windows, you should not explicitly link a separate math library (often referred to as 'libm' or 'm') [1][2]. The math functions required by the ISO C standard (such as sin, cos, sqrt, etc.) are provided directly by the Universal C Runtime (UCRT) library [3]. This runtime is automatically linked by MSVC by default [4][2]. Attempting to explicitly link a library named 'm' or 'm.lib' on Windows will typically result in a linker error (e.g., LNK1104: cannot open file 'm.lib'), as no such standalone library exists in the MSVC toolchain [1][5][6]. In CMake projects, you should ensure that 'm' is not added to your link dependencies when targeting MSVC [1][7]. A common, portable approach is to use a conditional check in your CMakeLists.txt to avoid linking 'm' on Windows [1][7][8]: # Example of portable math library linking in CMake if (NOT WIN32) target_link_libraries(your_target PUBLIC m) endif Alternatively, you can use the check_library_exists module to determine if a separate math library is required for your current compiler and platform, which correctly identifies that it is unnecessary on MSVC [8]. If you are working with cross-platform projects or third-party libraries (e.g., via pkg-config), you may occasionally encounter build scripts that incorrectly include '-lm' unconditionally; these should be patched to omit the dependency when the compiler is MSVC [9][6][10].
Citations:
- 1: https://stackoverflow.com/questions/54935559/linking-math-library-in-cmake-file-on-windows-and-linux
- 2: error checking math (m) library on windows mesonbuild/meson#1509
- 3: https://learn.microsoft.com/en-us/cpp/c-runtime-library/floating-point-support?view=msvc-170
- 4: https://learn.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=msvc-170
- 5: cmake: refined conditions for math library linking on windows ggml-org/llama.cpp#11312
- 6: [icu] ICU port adds -lm in pkgconfig files for debug MSVC builds microsoft/vcpkg#22311
- 7: robbr48/fmi4c@413ea2d
- 8: https://stackoverflow.com/questions/34625627/how-to-link-to-the-c-math-library-with-cmake
- 9: 3.3.9: CMake .pc file adds libm unconditonally FFTW/fftw3#236
- 10: [libjxl] Remove libm dependency when building with MSVC microsoft/vcpkg#46177
Guard libm linkage for non-MSVC toolchains.
Line 28 links m unconditionally; MSVC does not provide m.lib (math functions are included in UCRT), so this breaks Windows example builds when examples are enabled, resulting in linker error LNK1104.
Suggested fix
# Link against the C bindings
-target_link_libraries(CImg2NumExample_console_c PRIVATE CImg2Num m)
+target_link_libraries(CImg2NumExample_console_c PRIVATE CImg2Num)
+if(NOT MSVC)
+ target_link_libraries(CImg2NumExample_console_c PRIVATE m)
+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.
| target_link_libraries(CImg2NumExample_console_c PRIVATE CImg2Num m) | |
| target_link_libraries(CImg2NumExample_console_c PRIVATE CImg2Num) | |
| if(NOT MSVC) | |
| target_link_libraries(CImg2NumExample_console_c PRIVATE m) | |
| endif() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@example-apps/console-c/CMakeLists.txt` at line 28, The target_link_libraries
call for CImg2NumExample_console_c unconditionally links the math library `m`,
which is not available on Windows with MSVC toolchain and causes linker errors.
Guard the linking of the `m` library by wrapping it in a CMake conditional that
checks if the compiler is not MSVC (using if(NOT MSVC) or similar platform
detection), so that `m` is only linked on non-MSVC toolchains where the math
library is actually available.
|
@Krasner I can't find the comment you left about me renaming the example apps (I saw it in my emails, but clicking on the link just takes me to the commit on I changed their names to make future debugging a bit easier. Sometimes, you get really bad errors that don't really describe anything and you can't find whatever it is that they're describing. I did this mainly because I don't want the future possibility of a name collision or something similar. We should probably also do this for To sum it up: I'm lazy and want to be able to quickly scan error messages to figure out what's wrong. |
that's fine. we need to update the Justfile to reflect the new example app names |
I realised that and have done it in #452. |
2 participants
What was changed & why
Add strict CMake flags to avoid MSVC errors during builds.
Motivation: This workflow failure on the
devbranch.Changes
bindings/c/src/cimg2num.cppto conform to C++17