Skip to content

refactor: drop default arguments from virtual functions (google-default-arguments)#1488

Merged
annejan merged 1 commit into
mainfrom
refactor/no-defaults-on-virtuals
May 14, 2026
Merged

refactor: drop default arguments from virtual functions (google-default-arguments)#1488
annejan merged 1 commit into
mainfrom
refactor/no-defaults-on-virtuals

Conversation

@annejan
Copy link
Copy Markdown
Member

@annejan annejan commented May 14, 2026

Summary

Closes the loop on the deferred clang-tidy finding from #1487. Defaults on `virtual` / `override` methods are a Google-style anti-pattern because the default value depends on which static type the call goes through:

```cpp
// Base virtual with default
virtual void Move(QString src, QString dest, bool force = false) = 0;

// Override redeclares the default (or it would be a compile error)
void Move(QString src, QString dest, bool force = false) override;

Pass *base = new RealPass();
base->Move(a, b); // uses BASE default
realpass_ptr->Move(a, b); // uses RealPass default — may differ
```

If subclasses ever drift, the default value silently differs by pointer type. Remove the trap.

Changes

`src/pass.h` — remove defaults from 4 base virtuals:

  • `Move(..., bool force)`
  • `Copy(..., bool force)`
  • `Grep(..., bool caseInsensitive)`
  • `executeWrapper(..., bool readStdout, bool readStderr)`

`src/imitatepass.h` / `src/realpass.h` — strip the duplicated `= false` from every override declaration (`Insert`, `Remove`, `Move`, `Copy`, `Grep`, plus `executeWrapper` for ImitatePass).

Callers updated to pass values explicitly:

  • `src/storemodel.cpp`: 4× `Move`/`Copy` in drag-drop handlers.
  • `src/mainwindow.cpp`: 2× `Move` in `renameFolder` / `renamePassword`.
  • `src/pass.cpp`: 1× `executeWrapper` in `GenerateGPGKeys`.
  • `tests/auto/util/tst_util.cpp`: 2× `Grep`.
  • `tests/auto/integration/tst_integration.cpp`: 3× `Grep`, 1× `Move`, 1× `Copy`.

`.clang-tidy` — add `google-default-arguments` now that the codebase is clean. Locks the status quo in.

Test plan

  • `qmake6 -r && make -j4` clean
  • `clang-tidy --checks='-,google-default-arguments'` on `src/.cpp` → 0 findings
  • `tests/auto/util/tst_util` — 124/124
  • `tests/auto/integration/tst_integration` — 20/20
  • `clang-format` applied

Note

This is a refactor with semantic-preservation guarantees: every explicit `false` / `true, true` matches the previous implicit default exactly. No behavioural change.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Enhanced code quality by enforcing explicit parameter passing in internal methods, eliminating reliance on default values across password management operations and supporting utilities. Updated static analysis configuration to enforce consistency.

Review Change Stack

…lt-arguments)

Closes the loop on the deferred clang-tidy finding from #1487: defaults
on virtual / override methods are a Google-style anti-pattern because
the default value depends on which static type the call goes through.
Calling `pass_base_ptr->Move(a, b)` would use the base-class default;
calling `pass_derived_ptr->Move(a, b)` would be a compile error if the
override didn't redeclare it. The defaults silently make the value
depend on whether the caller has narrowed the pointer type.

QtPass's Pass interface had this on Move, Copy, Grep, and the
6-arg executeWrapper, with the defaults duplicated on the
ImitatePass/RealPass overrides. Removed from both sides; callers now
pass `false` (or `true,true` for executeWrapper readStdout/readStderr)
explicitly.

Production callsites updated:

- src/storemodel.cpp: 4× Move/Copy in handleDirDrop /
  handleFileToDirDrop — all rely on the force=false default for normal
  drag-drop semantics.
- src/mainwindow.cpp: 2× Move in renameFolder() / renamePassword() —
  same.
- src/pass.cpp: GenerateGPGKeys's executeWrapper call had relied on
  readStdout=true, readStderr=true defaults.

Test callsites updated (3 files):

- tests/auto/util/tst_util.cpp: 2× Grep("anything"/"[invalid").
- tests/auto/integration/tst_integration.cpp: 3× Grep("token"/"url"),
  plus Move/Copy.

Plus .clang-tidy: enable google-default-arguments now that the
codebase is clean.

Build clean, 124/124 tst_util pass, 20/20 tst_integration pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0645cd8e-c502-4954-a55e-43161981b96b

📥 Commits

Reviewing files that changed from the base of the PR and between dfee385 and b2ca8a1.

📒 Files selected for processing (9)
  • .clang-tidy
  • src/imitatepass.h
  • src/mainwindow.cpp
  • src/pass.cpp
  • src/pass.h
  • src/realpass.h
  • src/storemodel.cpp
  • tests/auto/integration/tst_integration.cpp
  • tests/auto/util/tst_util.cpp

📝 Walkthrough

Walkthrough

This PR removes default argument values from several Pass interface methods and updates all call sites to explicitly pass these arguments. The change enables the google-default-arguments clang-tidy check to enforce explicit defaults throughout the codebase. Affected methods are Move, Copy, Grep, and executeWrapper across abstract and concrete implementations, with corresponding updates to production code and tests.

Changes

Default argument removal

Layer / File(s) Summary
Abstract interface and tooling configuration
src/pass.h, .clang-tidy
Pass abstract base class signatures for Move, Copy, Grep, and executeWrapper remove their default boolean parameters. The .clang-tidy config is updated to enable google-default-arguments check to enforce this pattern.
Implementation class signatures
src/imitatepass.h, src/realpass.h
ImitatePass and RealPass override signatures are updated to match, removing defaults from Insert, Remove, Move, Copy, Grep, and executeWrapper to enforce explicit argument passing.
Production code call sites
src/pass.cpp, src/mainwindow.cpp, src/storemodel.cpp
Implementation calls in GenerateGPGKeys, folder/password rename operations, and drag-and-drop handlers are updated to pass explicit boolean values (false or true) instead of relying on defaults.
Test code call sites
tests/auto/integration/tst_integration.cpp, tests/auto/util/tst_util.cpp
Integration and unit tests are updated to explicitly pass false arguments when calling Grep, Move, and Copy through the test ImitatePass implementation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • IJHack/QtPass#1073: Modifies src/pass.cpp inside Pass::GenerateGPGKeys to refactor the gpgconf resolution used for key generation execution.
  • IJHack/QtPass#1066: Updates StoreModel drag-and-drop handling in storemodel.cpp/storemodel.h where drop dispatch and Move/Copy calls are adjusted.

Suggested labels

size:L

Poem

🐰 Default args fade to dust,
Explicit calls now we trust,
Clang-tidy pleased, no more unsaid,
Every boolean now properly fed!
Clear intentions, front and center bright,
QtPass compiles with linting might!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main refactoring objective—removing default arguments from virtual functions to satisfy the google-default-arguments clang-tidy check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/no-defaults-on-virtuals

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.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage is 44.752%refactor/no-defaults-on-virtuals into main. No base build found for main.

@annejan annejan merged commit d29ab23 into main May 14, 2026
24 of 25 checks passed
@annejan annejan deleted the refactor/no-defaults-on-virtuals branch May 14, 2026 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants