Skip to content

DOC: Add compiler-cautions.md — compiler-specific pitfalls from 10 years of COMP: commits#5999

Merged
hjmjohnson merged 3 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:common-code-pitfals-md
Apr 2, 2026
Merged

DOC: Add compiler-cautions.md — compiler-specific pitfalls from 10 years of COMP: commits#5999
hjmjohnson merged 3 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:common-code-pitfals-md

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented Apr 1, 2026

Summary

Adds Documentation/AI/compiler-cautions.md, a reference document for AI agents
and human contributors cataloguing compiler-specific pitfalls and refactoring
hazards distilled from:

  • 1,296+ COMP: commits (2019–2026)
  • GitHub PR review comments (open and closed)
  • CDash nightly build failure analysis

Adds a routing entry to the AGENTS.md context table so agents load this
document when working on refactoring or cross-platform fixes.

Document Coverage (12 sections)

Section Key pitfalls
Smart pointers to forward-declared types GCC 7–9.1 {} brace-init bug (PR #3877); destructor-in-header UB with unique_ptr<Incomplete> (PR #5997)
Template deduction Missing typename (AppleClang 12+); non-type param type mismatch int/unsigned int; constexpr static ODR-use in lambdas (GCC 7)
constexpr pitfalls MSVC x86 if constexpr with runtime sub-expressions; constexpr array of function pointers
Undefined behavior Bit shift overflow; implicit integer truncation (UBSan); data() on empty std::vector; uninitialized loop vars
Platform / architecture char signedness on ARM Linux; missing <cstdint> on GCC 13+; reserved identifiers on AppleClang
GCC-specific -Wmaybe-uninitialized false positives on ARM64 (GCC 11+); -Wdeprecated-copy Rule of Zero
MSVC-specific C4805 bool |= int; constexpr function pointer arrays
Clang/AppleClang -Wzero-as-null-pointer-constant in third-party headers; -Wduplicate-enum; -Wunused-lambda-capture
ITK API deprecations itkTypeMacro, ITK_DISALLOW_COPY_AND_ASSIGN, itkStaticConstMacro — hard errors with ITK_LEGACY_REMOVE=ON
Shared library visibility Missing ITK_TEMPLATE_EXPORT on explicit instantiations
Python bindings np.bool (NumPy 1.24+); PySequence_Fast_GET_ITEM (Python 3.14)
Refactoring checklist 13-item checklist to verify before submitting a refactoring PR

AI Assistance Disclosure

Research and document structure generated with Claude Code / claude-sonnet-4-6.
All pitfall entries are grounded in specific ITK commits, PR numbers, and CDash
build configurations cited in the document. Human author reviewed entries for
accuracy before committing.

Test plan

  • Documentation only — no build or test changes
  • Verify compiler-cautions.md renders correctly on GitHub
  • Verify AGENTS.md routing table link is correct

🤖 Generated with Claude Code (claude-sonnet-4-6)

@github-actions github-actions bot added type:Documentation Documentation improvement or change area:Documentation Issues affecting the Documentation module labels Apr 1, 2026
@hjmjohnson hjmjohnson requested a review from N-Dekker April 1, 2026 00:59
@hjmjohnson hjmjohnson self-assigned this Apr 1, 2026
@hjmjohnson hjmjohnson force-pushed the common-code-pitfals-md branch 3 times, most recently from baf6655 to e1085de Compare April 1, 2026 13:02
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Generally looks good. I did not read the first commit thoroughly. Someone else should review this.

Comment thread Documentation/AI/compiler-cautions.md Outdated
@hjmjohnson hjmjohnson force-pushed the common-code-pitfals-md branch from e1085de to d0e0fbf Compare April 1, 2026 16:10
Comment thread Documentation/AI/compiler-cautions.md Outdated
hjmjohnson and others added 2 commits April 1, 2026 14:44
- Disable llvmlibc-* (LLVM stdlib internals, never applicable to ITK)
- Disable modernize-use-trailing-return-type (ITK uses leading return types)
- Disable readability-redundant-member-init (ITK prefers explicit base-class init)
- Expand readability-identifier-length.IgnoredParameterNames to cover
  single-letter index/coordinate params (i,j,k,n,x,y,z,r,g,b,a,v)
  commonly used in ITK math-heavy code

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Distilled from review of 1,296+ COMP: commits (2019-2026), GitHub PR
review comments, and CDash failure analysis. Covers:

- Smart pointer / forward-declared type hazards (GCC 7-9.1 brace-init
  bug, destructor-in-header UB with unique_ptr<Incomplete>)
- Template deduction pitfalls (missing typename, non-type param type
  mismatch, constexpr static ODR-use in lambdas)
- constexpr edge cases (MSVC x86 if constexpr, function pointer arrays)
- Undefined behavior patterns (bit shift overflow, implicit truncation,
  empty-vector data(), uninitialized loop vars)
- Platform specifics (char signedness on ARM, missing <cstdint> on
  GCC 13+, reserved identifiers on AppleClang)
- GCC warnings (-Wmaybe-uninitialized false positives on ARM64,
  -Wdeprecated-copy Rule of Zero violations)
- MSVC C4805 bool/int mixing, constexpr arrays
- Clang suppression macros for third-party headers
- ITK deprecated API replacements (ITK_LEGACY_REMOVE=ON breaks)
- Shared library symbol visibility (ITK_TEMPLATE_EXPORT)
- Python binding removals (np.bool, PySequence_Fast_GET_ITEM)
- 13-item refactoring checklist

Add routing entry to AGENTS.md table.

Document that refactoring should not introduce new clang-tidy diagnostics,
and enumerate check families that conflict with ITK coding standards
(llvmlibc-*, readability-identifier-length, google-*, etc.).

AI agents must consult compiler-cautions.md, compile the changed
module, and run relevant tests before committing C++ code.
Update AGENTS.md routing row for commit tasks to surface these docs.
@hjmjohnson hjmjohnson force-pushed the common-code-pitfals-md branch from 4de9274 to 08a59b5 Compare April 1, 2026 19:45
@hjmjohnson hjmjohnson marked this pull request as ready for review April 1, 2026 19:47
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 1, 2026

Greptile Summary

This PR adds Documentation/AI/compiler-cautions.md, a reference document cataloguing compiler-specific pitfalls distilled from ITK's COMP: commit history, and wires it into the AGENTS.md context-routing table and the git-commits.md pre-commit checklist. The .clang-tidy file is updated to suppress three check families that are documented as incompatible with ITK conventions.

Changes overview:

  • New compiler-cautions.md (541 lines, 12 categories, ~13 subsections) with concrete BAD/GOOD code examples and ITK PR/commit citations.
  • AGENTS.md updated: new "Avoiding compiler-specific pitfalls" routing row; "Creating a git commit" row expanded to bundle compiler, build, and test references.
  • git-commits.md gains a three-step pre-commit checklist directing AI agents to review cautions, build, and test before committing.
  • .clang-tidy disables llvmlibc-*, modernize-use-trailing-return-type, and readability-redundant-member-init; broadens IgnoredParameterNames for common scientific single-character identifiers.

Issues found:

  • P1 — PySequence_GetItem example missing null guard (§11b): PySequence_GetItem returns NULL on error; calling Py_DECREF(NULL) is UB and will crash. The "GOOD" snippet should include if (item != nullptr) before using and decrementing the reference count.
  • P2 — Section numbering out of order: ## 13. clang-tidy Warnings During Refactoring appears physically before ## 12. Quick-Reference Checklist for Refactoring in the document body; the two sections should be reordered or renumbered for consistency.

Confidence Score: 4/5

  • Safe to merge after fixing the PySequence_GetItem null-check example; the null omission in the reference snippet could propagate to real code copied by contributors.
  • All four files are documentation/configuration only with no build or runtime impact on ITK itself. The P1 finding (missing null guard in §11b) is in an example code block that may be copied verbatim by AI agents or human contributors, creating a real crash-risk if replicated; it should be fixed before merge. The section-ordering issue (P2) is cosmetic.
  • Documentation/AI/compiler-cautions.md — section 11b PySequence_GetItem example and sections 12/13 ordering.

Important Files Changed

Filename Overview
Documentation/AI/compiler-cautions.md New 541-line reference document covering 12 compiler-pitfall categories; two issues: sections 12 and 13 appear out of order in the file body, and the PySequence_GetItem "GOOD" example omits the mandatory null check.
AGENTS.md Adds a routing row for compiler-cautions.md and expands the "Creating a git commit" row to bundle compiler, build, and test references; both changes are consistent with the new document.
.clang-tidy Disables three check families documented as ITK-incompatible (llvmlibc-*, modernize-use-trailing-return-type, readability-redundant-member-init) and broadens IgnoredParameterNames to include common scientific-computing single-char identifiers; fully aligned with compiler-cautions.md §13b.
Documentation/AI/git-commits.md Appends a three-step pre-commit checklist (review cautions, compile, test) with correct relative links to compiler-cautions.md, building.md, and testing.md; clean addition with no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[AI agent / human contributor\nbegins C++ work] --> B{Task type?}
    B -- Refactoring / cross-platform fix --> C[Load compiler-cautions.md\nAGENTS.md routing]
    B -- Writing new code --> D[Load conventions.md\nstyle.md]
    C --> E[Check Quick-Ref Checklist §12]
    D --> E
    E --> F[Write / modify C++ code]
    F --> G[Pre-commit checklist\ngit-commits.md §Pre-Commit]
    G --> G1[1. Re-read compiler-cautions.md]
    G --> G2[2. Build affected module\nbuilding.md]
    G --> G3[3. Run targeted tests\nctest -R pattern]
    G1 & G2 & G3 --> H[git commit]
    H --> I[Open PR → pull-requests.md]
Loading

Reviews (1): Last reviewed commit: "DOC: Add compiler-cautions.md with pitfa..." | Re-trigger Greptile

Comment thread Documentation/AI/compiler-cautions.md Outdated
Comment thread Documentation/AI/compiler-cautions.md Outdated
- §11b: add mandatory null check before Py_DECREF after
  PySequence_GetItem — omitting it is UB if the call returns NULL
- Renumber §12/§13: clang-tidy section was numbered 13 but appeared
  before §12 (Checklist) in the file; renumber to match physical order
Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Most excellent!

@hjmjohnson hjmjohnson merged commit b94cca5 into InsightSoftwareConsortium:main Apr 2, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Documentation Issues affecting the Documentation module type:Documentation Documentation improvement or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants