Refresh stale content in copilot-instructions.md - #2049
Merged
Merged
Conversation
Several sections still described the pre-OpenCvSharp5 / pre-StdVector<T> state of the repo: the NuGet README sync table listed OpenCvSharp4.* package names (actual PackageIds are all OpenCvSharp5.* now, including the renamed GdipExtensions and the new AvaloniaExtensions package), the free-function facade scope list was missing Shape/VideoIORegistry, the std::vector guidance still pointed at VectorOfInt32/VectorOfVec4f/ VectorOfVec6d (none of which exist anymore now that blittable element types use the generic StdVector<T> directly), the EdgeDrawing reference description and Params-struct code sample still showed the old VectorOfVec6d / [MarshalAs(UnmanagedType.Bool)] patterns instead of the current StdVector<T> / plain-int-field conventions, and the branch note implied a separate "5.x" branch exists alongside main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughUpdated ChangesOpenCvSharp5 contributor guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Add several conventions/pitfalls that came up repeatedly across past sessions but were only recorded informally, so they're now discoverable by anyone (human or AI) editing this repo: - Explicit argument-validation rule (ArgumentNullException.ThrowIfNull etc., with the three cases that stay unconverted). - POD value types crossing the extern boundary go in `interop::` with a bit_cast-based converter, with its two exceptions. - A "one-shot config classes" alternative to the per-field Params struct pattern (getAll/setAll + blittable POD). - Common P/Invoke pitfalls: cv::Ptr<T>* vs. raw T* mismatches, silent LPStr/LPUTF8Str marshaling regressions, and testing a round trip before copying an existing binding pattern. - A native-ABI/breaking-change policy section (5.x allows changing both sides, but prefer the smallest natural fix), folding in the established "justify deletions with a BCL alternative" and "don't add IDisposable for a minor efficiency win" judgment calls. - Markdown authoring (no hard-wrapping mid-paragraph) and pull request conventions (match existing merged-PR style, no invented headings). - Explicit "all repository content is English" statement, and the Extensions packages (Wpf/Gdip/Avalonia) in the repository-structure overview. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/copilot-instructions.md:
- Around line 178-182: Update the wording in the first “Common P/Invoke
pitfalls” bullet to clearly state that the incorrect pointer declaration broke
the BackgroundSubtractorGMG/MOG property accessors, replacing the confusing
“this bit” phrasing without changing the technical guidance.
🪄 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: CHILL
Plan: Pro
Run ID: fce2aee4-be7c-495a-a7ce-fb9a3a8bbecb
📒 Files selected for processing (1)
.github/copilot-instructions.md
Comment on lines
+178
to
+182
| ### Common P/Invoke pitfalls | ||
|
|
||
| - **`cv::Ptr<T>*` vs. raw `T*` confusion.** `CvPtrObject.Handle` (used by most property getters/setters) returns a raw `T*`, not the smart pointer — a native binding declared as `cv::Ptr<T>* obj` with `(*obj)->getXxx()` silently misinterprets the raw pointer's vtable as a `cv::Ptr`'s internal layout and reliably crashes (access violation) the first time it's called, not at compile time. Match the parameter type to what the C# side actually passes (`Handle` → raw `T*` parameter with `obj->getXxx()`); this bit `BackgroundSubtractorGMG`/`MOG`'s property accessors and went unnoticed because no test exercised a round trip. | ||
| - **Don't change a marshaling attribute (`[MarshalAs(UnmanagedType.LPStr)]` vs. `LPUTF8Str`) without checking every native code path that consumes it.** Different OpenCV backends decode filename strings differently (e.g. `cap_msmf.cpp` uses `MultiByteToWideChar(CP_ACP, ...)`, i.e. it expects ANSI, not UTF-8) — switching the attribute to "fix" one call site can silently corrupt non-ASCII paths on a different backend. This kind of regression doesn't show up in a plain build/test run; it only surfaces when you actually exercise the affected path with non-ASCII input. | ||
| - **Write one real get/set (or call/verify) round trip before copying an existing property/method pattern to a new class.** Don't trust an existing, untested binding as a copy-source just because it compiles — verify it actually works first. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the wording in the P/Invoke pitfall.
“this bit BackgroundSubtractorGMG/MOG's property accessors” should read “this broke the BackgroundSubtractorGMG/MOG property accessors” (or equivalent); the current wording is confusing in guidance about a crash-prone binding error.
🤖 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 @.github/copilot-instructions.md around lines 178 - 182, Update the wording
in the first “Common P/Invoke pitfalls” bullet to clearly state that the
incorrect pointer declaration broke the BackgroundSubtractorGMG/MOG property
accessors, replacing the confusing “this bit” phrasing without changing the
technical guidance.
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Staleness fixes — several sections of
.github/copilot-instructions.mdstill described the pre-OpenCvSharp5 / pre-StdVector<T>state of the repo:OpenCvSharp4,OpenCvSharp4.Windows,OpenCvSharp4.Windows.Slim,OpenCvSharp4.Extensions,OpenCvSharp4.WpfExtensions,OpenCvSharp4.runtime.*,OpenCvSharp4.official.runtime.*— but every currentPackageId(checked across all.csprojfiles) isOpenCvSharp5.*. Also missing:OpenCvSharp4.Extensionswas renamed toOpenCvSharp5.GdipExtensions, and a newOpenCvSharp5.AvaloniaExtensionspackage exists.ShapeandVideoIORegistry, which already haveCv2.Shape.cs/Cv2.VideoIORegistry.csfacades.std::vectorreturn values guidance: prescribedVectorOfInt32,VectorOfVec4f,VectorOfVec6d— none of which exist anymore. Blittable/primitive element types now use the genericStdVector<T>directly; dedicatedVectorOfXxxclasses are only for non-blittable/nested types.VectorOfVec6d; it now usesStdVector<Vec6d>/StdVector<Vec4f>/StdVector<int>.[MarshalAs(UnmanagedType.Bool)]on the P/Invoke struct — current practice uses a plainintfield with manual conversion instead.`main`/`5.x`as if a separate5.xbranch exists; onlymainand4.xactually exist.New content — transcribed several conventions and pitfalls that had only been recorded informally across past sessions, so they're now discoverable by anyone (human or AI) editing this repo:
ArgumentNullException.ThrowIfNulletc., with the three cases that intentionally stay unconverted).interop::with abit_cast-based converter, plus its two exceptions.Paramsstruct pattern (getAll/setAll+ blittable POD).cv::Ptr<T>*vs. rawT*mismatches, silentLPStr/LPUTF8Strmarshaling regressions, and testing a round trip before copying an existing binding pattern.IDisposablefor a minor efficiency win."No functional/code changes — documentation only.
Summary by CodeRabbit
mainbranch behavior, and 4.x freeze policy.std::vector/VectorOf*types, including when to useStdVector<T>and when to generate specialized vector wrappers.