Skip to content

STYLE: Remove unused non-trivial variables (clazy unused-non-trivial-variable)#1374

Merged
jamesobutler merged 1 commit intocommontk:masterfrom
BRAINSia:pr/unused-nontrivial-variable
Mar 28, 2026
Merged

STYLE: Remove unused non-trivial variables (clazy unused-non-trivial-variable)#1374
jamesobutler merged 1 commit intocommontk:masterfrom
BRAINSia:pr/unused-nontrivial-variable

Conversation

@hjmjohnson
Copy link
Copy Markdown
Contributor

@hjmjohnson hjmjohnson commented Mar 24, 2026

Cherry-picked from #1372, part of a systematic clazy static analysis effort to improve CTK for Slicer compatibility and Qt6 migration.

What this changes

Removes local variables of non-trivial Qt types (QStringList, QList, etc.) that are declared but never read or used. These trigger constructors and destructors with no effect — dead code flagged by the clazy unused-non-trivial-variable check.

Why

Dead code — these objects are constructed and destroyed without being read or used. Removing them reduces noise and slightly reduces binary size.

Testing

  • Builds cleanly with zero new compiler warnings
  • Existing tests pass (no regressions)

🤖 Identified via clazy static analysis using CTK-claude-skills

@hjmjohnson hjmjohnson changed the title STYLE STYLE: Remove unused non-trivial variables (clazy unused-non-trivial-variable) Mar 24, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review March 26, 2026 13:34
@hjmjohnson
Copy link
Copy Markdown
Contributor Author

@jamesobutler A little cleanup while getting Qt6 support fortified.

Comment thread Libs/DICOM/Core/ctkDICOMDatabase.cpp
@hjmjohnson
Copy link
Copy Markdown
Contributor Author

@jamesobutler — good catch. The TODO documents that DCM_PatientAge should eventually be stored at the study level: it records age at time of study, not a fixed patient-level attribute per the DICOM standard.

I've opened #1380 to track moving it to the Studies table properly. The patientsAge variable is correctly removed here since it is genuinely unused (the bindValue calling it is already commented out and the NULL insert is what the current schema does).

@jamesobutler
Copy link
Copy Markdown
Contributor

Could you rebase this branch instead to pull out the merge commit and allow the commit message check to pass?

@hjmjohnson
Copy link
Copy Markdown
Contributor Author

Could you rebase this branch instead to pull out the merge commit and allow the commit message check to pass?

Yes. NOTE:
image

Can we change the project settings so that rebase is the default behavior? Perhaps prevent merges.

…variable)

Remove unused Qt value-type variables (QString, QDir, QStringList, QVariant,
QMap, QList) detected by clazy. Most were dead code from debugging or
refactoring. Also fix a bug in ctkDICOMDatabase::cleanup() where
tagcacheCleanup was created for the TagCacheDatabase but seriesCleanup
was mistakenly called instead, so the tag cache was never VACUUM'd.

Issue created for tracking commented out variable that needs to be
moved.  commontk#1380

AI tools used to identify and fix these warnings
@hjmjohnson hjmjohnson force-pushed the pr/unused-nontrivial-variable branch from dcec32a to 524e729 Compare March 28, 2026 15:55
@jamesobutler
Copy link
Copy Markdown
Contributor

From my user role, I unfortunately don’t have the options in settings for this repo to make those changes.

@jamesobutler jamesobutler enabled auto-merge (rebase) March 28, 2026 16:00
@jamesobutler jamesobutler merged commit 41b92ec into commontk:master Mar 28, 2026
4 checks passed
seriesCleanup.exec("VACUUM;");
QSqlQuery tagcacheCleanup(d->TagCacheDatabase);
seriesCleanup.exec("VACUUM;");
tagcacheCleanup.exec("VACUUM;");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch! I noticed the lack of cleaning up of tagcache file but never had the time to investigate.

@lassoan
Copy link
Copy Markdown
Member

lassoan commented Mar 30, 2026

Can we change the project settings so that rebase is the default behavior? Perhaps prevent merges.

I dislike the "Update with merge commit" option, too, but I don't think we can disable it or change the default. To me, it seems that the default is a user-specific setting (probably for each repository, always set to using merge commit by default).

image

These are the pull request settings for this repository. Let me know if you have any suggestion on what to change to remove the merge commit option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants