Repository navigation
Fix ChildLocker copy ctor deletion. - #380
Conversation
|
No actionable comments were generated in the recent review.
 |
| Author | Lines added | Lines removed |
|---|---|---|
| Rene Ferdinand Rivera Morell | 1 | 1 |
The deleted ChildLocker constructor now takes a const ChildLocker & instead of a const Locker &.
Changes: ChildLocker Copy Constructor
| Layer / File(s) | Summary |
|---|---|
Correct the deleted constructor MarshallerThread.h |
The deleted constructor declaration now takes a const ChildLocker &. |
Priority:
Estimated code review effort: 1 (Trivial) | ~2 minutes
Change: Bug fix
Possibly related PRs
- OPENDAP/libdap4#381: Adds ABI comparison checks that can assess the impact of this public constructor declaration change.
Merge Risk: 
c0f76
-
This declaration-only fix is ready to merge; no concrete ABI or runtime risk is identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title is concise, specific, and accurately describes the correction to the ChildLocker copy-constructor deletion. Description check The description explains the defect, the correction, and the linked issue. It includes the relevant task checklist, although it does not include the template's explicit Reference ticket field or test … Linked Issues check Issue #377requires that the ChildLocker copy constructor be deleted, with ChildLocker as the argument type, matching ldap::Locker. The whole-PR diff (mergeBase 90bed3f to head c0f76d3) changes Marsha…Out of Scope Changes check The whole-PR diff contains one hunk in MarshallerThread.h, a single-line change to the deleted copy-constructor declaration of ChildLocker. This hunk implements #377directly. The file list at head sh…Full details: Description check
-
Explanation
The description explains the defect, the correction, and the linked issue. It includes the relevant task checklist, although it does not include the template's explicit Reference ticket field or test checklist item.
Full details: Linked Issues check
-
Explanation
Issue
#377requires that the ChildLocker copy constructor be deleted, with ChildLocker as the argument type, matching ldap::Locker. The whole-PR diff (mergeBase 90bed3f to head c0f76d3) changes MarshallerThread.h line 76 fromChildLocker(const Locker &rhs) = delete;toChildLocker(const ChildLocker &rhs) = delete;. This is the coding change the issue asks for. The PR adds no test. The issue asks for no test, so no test requirement applies. The PR author's ABI question is a review comment, not an issue requirement, so it does not affect this verdict.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff contains one hunk in MarshallerThread.h, a single-line change to the deleted copy-constructor declaration of ChildLocker. This hunk implements
#377directly. The file list at head shows no other changed paths in the reviewed diff output. The diff output was truncated at 16000 bytes, so the full file list was not read. The diff returned only the MarshallerThread.h hunk, so no additional changed hunks were observed in the returned output.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the header line,
The copy rule is now aligned.
A ChildLocker cannot copy,
Its deleted constructor says so plainly.
The rabbit hops away, content.
Comment @coderabbitai help to get the list of available commands.
|
Is the ABI change in this a concern? |
While refactoring the copy ctor for ChildLocker was attempted declared but the given argument was given the type of the superclass instead of the subclass. This was not an issue as we never try and copy these. This change corrects the argument to actually delete the copy ctor (which prevents the other copy ctors from being generated). Fixes #377
3473520 to
c0f76d3
Compare
|
Fix ChildLocker copy ctor deletion. (#377)
Description
While refactoring the copy ctor for ChildLocker was attempted declared but the given argument was given the type of the superclass instead of the subclass. This was not an issue as we never try and copy these. This change corrects the argument to actually delete the copy ctor (which prevents the other copy ctors from being generated).
** Fixes #377 **
Tasks