Skip to content

STYLE: Replace non-POD string constants with constexpr QLatin1String - #1410

Closed
hjmjohnson wants to merge 1 commit into
commontk:masterfrom
BRAINSia:fix-non-pod-constexpr-strings
Closed

STYLE: Replace non-POD string constants with constexpr QLatin1String#1410
hjmjohnson wants to merge 1 commit into
commontk:masterfrom
BRAINSia:fix-non-pod-constexpr-strings

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Apr 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes clazy:non-pod-global-static warnings on file-scope and class-static string constants by converting them to static constexpr QLatin1String.

This is one of two PRs split from a combined approach (the companion PR #1409 handles ctkLogger globals via Q_GLOBAL_STATIC_WITH_ARGS).

Problem

static QString at file scope or as class static members have non-trivial constructors and destructors, triggering the non-pod-global-static clazy warning. These are subject to the Static Initialization Order Fiasco (SIOF) and add unnecessary startup/teardown overhead.

Approach

QLatin1String is a lightweight wrapper around a const char* + length — no heap allocation, trivially constructible/destructible. All affected constants are pure compile-time string literals, making them ideal constexpr candidates.

Qt5 / Apple-clang compatibility: QLatin1String(const char*) calls strlen(), which Apple clang does not treat as constexpr. The 2-argument constructor with sizeof("literal") - 1 is used instead:

// Before (triggers non-pod-global-static):
static QString PREFIX_EXECUTABLE = "executable:";

// After (zero runtime overhead, compile-time constant):
static constexpr QLatin1String PREFIX_EXECUTABLE{
    "executable:", static_cast<int>(sizeof("executable:") - 1)};

For class static members (ctkPluginFrameworkDebug), the constexpr definition moves entirely into the header (C++17 static constexpr is implicitly inline), the out-of-line QString definitions in the .cpp are removed, and the CTK_OSGI intermediate variable becomes unnecessary:

// Before — header declaration + .cpp definition built from a runtime prefix:
// ctkPluginFrameworkDebug_p.h:  static QString OPTION_DEBUG_GENERAL;
// ctkPluginFrameworkDebug.cpp:  QString ctkPluginFrameworkDebug::OPTION_DEBUG_GENERAL = CTK_OSGI + "/debug";

// After — header-only compile-time constant:
static constexpr QLatin1String OPTION_DEBUG_GENERAL{
    "org.commontk.pluginfw/debug",
    static_cast<int>(sizeof("org.commontk.pluginfw/debug") - 1)};

No call-site changes required — QLatin1String implicitly converts to QString for all existing uses.

Files Changed

File Change
Libs/CommandLineModules/Frontend/QtGui/ctkCmdLineModuleObjectTreeWalker.cpp 4 prefix constants
Libs/PluginFramework/ctkBasicLocation.cpp 2 constants
Libs/PluginFramework/ctkLocationManager.cpp 11 constants
Libs/PluginFramework/ctkPluginFrameworkDebug_p.h 10 class static members → constexpr inline
Libs/PluginFramework/ctkPluginFrameworkDebug.cpp Remove 10 out-of-line definitions + CTK_OSGI variable
Libs/PluginFramework/ctkPluginFrameworkLauncher.cpp 1 constant

Comparison with PR #1398

Aspect PR #1398 This PR
String constants Meyers' singleton const T& name() constexpr QLatin1String name
Call sites Need () suffix No change
Runtime overhead Thread-safe guard on each access None (compile-time const)
SIOF risk Eliminated Eliminated

Testing

Built successfully against Qt5 (cmake-build-clazy-qt5, Apple clang) and Qt6 (cmake-build-clazy-qt6) on macOS ARM64.

Part of the fix for #1407. See also companion PR #1409 for Q_GLOBAL_STATIC_WITH_ARGS logger fixes, and draft PR #1411 for the ctkDICOMModalities API deprecation strategy.

🤖 Generated with Claude Code

Static QString variables have non-trivial constructors and destructors,
triggering clazy:non-pod-global-static and introducing Static
Initialization Order Fiasco (SIOF) risk.  All affected constants are
pure compile-time string literals convertible to constexpr QLatin1String.

Qt5 / Apple-clang compatibility: QLatin1String(const char*) calls
strlen(), which is not constexpr under Apple clang.  A local
make_constexpr_QLatin1String_ helper in each anonymous namespace deduces
the string length at compile time via a const-array-reference parameter,
avoiding sizeof/static_cast repetition at every definition site:

  template<std::size_t N>
  constexpr QLatin1String make_constexpr_QLatin1String_(const char (&str)[N])
  { return QLatin1String{str, static_cast<int>(N - 1)}; }

The redundant 'static' specifier is omitted on all anonymous-namespace
members (internal linkage is already implied by the enclosing namespace).

For ctkPluginFrameworkDebug, the ten OPTION_DEBUG_* class static members
are converted to inline static constexpr in the private header using the
helper and a #define CTK_OSGI prefix macro (scoped with #undef); the
out-of-line QString definitions and CTK_OSGI QString variable are removed
from the .cpp entirely.  No call-site changes are required -- QLatin1String
implicitly converts to QString for all existing uses.

Files changed:
- Libs/CommandLineModules/Frontend/QtGui/ctkCmdLineModuleObjectTreeWalker.cpp
- Libs/PluginFramework/ctkBasicLocation.cpp
- Libs/PluginFramework/ctkLocationManager.cpp
- Libs/PluginFramework/ctkPluginFrameworkDebug_p.h
- Libs/PluginFramework/ctkPluginFrameworkDebug.cpp
- Libs/PluginFramework/ctkPluginFrameworkLauncher.cpp

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@hjmjohnson
hjmjohnson marked this pull request as draft April 2, 2026 20:34
@hjmjohnson

Copy link
Copy Markdown
Contributor Author

CTK Validation Report for PR #1410

Date: 2026-04-03T13:37:14Z
Platform: Linux 6.17.0-1012-oem x86_64

Role CTK Hash Commit
Proposed 1e017029 1e01702 (HEAD -> pr-1410) STYLE: Replace non-POD string constants with constexpr QLatin1String
Reference 935c4e04 935c4e0 (HEAD) ENH: Add refreshBrowser to the DICOM visual browser

Validation Actions Performed

# Action Scope Qt Proposed (1e017029) Reference (935c4e04)
1 Build CTK standalone Qt5 0 errors, 0 warnings 0 errors, 21 warnings
2 Build CTK standalone Qt6 0 errors, 0 warnings 2 errors, 3 warnings
3 Test CTK standalone Qt5 267/280 passed, 13 failed 255/280 passed, 25 failed
4 Test CTK standalone Qt6 245/255 passed, 10 failed 245/255 passed, 10 failed
5 Build CTK-in-Slicer (clean) Qt5 0 errors, 187 warnings 0 errors, 187 warnings
6 Build Slicer inner build Qt5 0 errors, 0 warnings 0 errors, 0 warnings
7 Test Slicer inner build Qt5 661/670 passed, 9 failed 661/670 passed, 9 failed

CTK-in-Slicer Build Detail (classified warnings)

Source Proposed errors Proposed warnings Reference errors Reference warnings
CTK source code 0 1 0 1
CTK dependencies 0 186 0 186
Slicer (CTK-related) 0 0 0 0
Slicer (own code) 0 0 0 0

Slicer Test Detail (classified failures)

Category Proposed (1e017029) Reference (935c4e04)
CTK-related failures 0 0
Slicer-own failures 0 0
Total failed 9 9
CTK Test Failures (proposed 1e017029)

Qt5 (13 failures):

	 94 - ctkFontButtonTest (Timeout)                       CTKWidgets
	 96 - ctkLanguageComboBoxTest (Failed)                  CTKWidgets
	144 - ctkWidgetsUtilsTestGrabWidget (Failed)            CTKWidgets
	158 - ctkColorPickerButtonEventTranslatorPlayerTest1 (Timeout) CTKWidgets
	162 - ctkDirectoryButtonEventTranslatorPlayerTest1 (Timeout) CTKWidgets
	166 - ctkFileDialogEventTranslatorPlayerTest1 (Timeout) CTKWidgets
	170 - ctkMenuComboBoxEventTranslatorPlayerTest1 (Failed) CTKWidgets
	236 - vtkLightBoxRendererManagerTest1 (Subprocess aborted) CTKVisualizationVTKCore
	269 - ctkVTKMagnifyViewTest2OddOdd (Failed)             CTKVisualizationVTKWidgets
	270 - ctkVTKMagnifyViewTest2EvenEven (Failed)           CTKVisualizationVTKWidgets
	271 - ctkVTKMagnifyViewTest2OddEven (Failed)            CTKVisualizationVTKWidgets
	272 - ctkVTKMagnifyViewTest2EvenOdd (Failed)            CTKVisualizationVTKWidgets
	280 - ctkVTKTextPropertyWidgetEventTranslatorPlayerTest1 (Timeout) CTKVisualizationVTKWidgets

Qt6 (10 failures):

	 38 - CTKPluginFrameworkAppTests (Subprocess aborted)   CTKPluginFramework
	 99 - ctkLanguageComboBoxTest (Failed)                  CTKWidgets
	147 - ctkWidgetsUtilsTestGrabWidget (Failed)            CTKWidgets
	148 - ctkWorkflowWidgetTest1 (Failed)                   CTKWidgets
	149 - ctkWorkflowWidgetTest2 (Failed)                   CTKWidgets
	206 - vtkLightBoxRendererManagerTest1 (SEGFAULT)        CTKVisualizationVTKCore
	249 - ctkVTKMagnifyViewTest2OddOdd (Failed)             CTKVisualizationVTKWidgets
	250 - ctkVTKMagnifyViewTest2EvenEven (Failed)           CTKVisualizationVTKWidgets
	251 - ctkVTKMagnifyViewTest2OddEven (Failed)            CTKVisualizationVTKWidgets
	252 - ctkVTKMagnifyViewTest2EvenOdd (Failed)            CTKVisualizationVTKWidgets
CTK Test Failures (reference 935c4e04)

Qt5 (25 failures):

	  7 - ctkBackTraceTest (Failed)                         CTKCore
	 20 - ctkLoggerTest1 (Failed)                           CTKCore
	 94 - ctkFontButtonTest (Timeout)                       CTKWidgets
	 96 - ctkLanguageComboBoxTest (Failed)                  CTKWidgets
	109 - ctkMessageBoxDontShowAgainTest (Failed)           CTKWidgets
	144 - ctkWidgetsUtilsTestGrabWidget (Failed)            CTKWidgets
	158 - ctkColorPickerButtonEventTranslatorPlayerTest1 (Timeout) CTKWidgets
	162 - ctkDirectoryButtonEventTranslatorPlayerTest1 (Failed) CTKWidgets
	166 - ctkFileDialogEventTranslatorPlayerTest1 (Timeout) CTKWidgets
	170 - ctkMenuComboBoxEventTranslatorPlayerTest1 (Failed) CTKWidgets
	173 - ctkPathLineEditEventTranslatorPlayerTest1 (Failed) CTKWidgets
	224 - ctkDICOMPatientDelegateTest1 (Failed)             CTKDICOMWidgets
	236 - vtkLightBoxRendererManagerTest1 (SEGFAULT)        CTKVisualizationVTKCore
	242 - ctkVTKErrorLogModelFileLoggingTest1 (Failed)      CTKVisualizationVTKWidgets
	246 - ctkVTKHistogramTest3 (Failed)                     CTKVisualizationVTKWidgets
	250 - ctkVTKPropertyWidgetTest (Failed)                 CTKVisualizationVTKWidgets
	253 - ctkVTKThresholdWidgetTest1 (Failed)               CTKVisualizationVTKWidgets
	263 - ctkVTKSurfaceMaterialPropertyWidgetTest1 (Failed) CTKVisualizationVTKWidgets
	269 - ctkVTKMagnifyViewTest2OddOdd (Failed)             CTKVisualizationVTKWidgets
	270 - ctkVTKMagnifyViewTest2EvenEven (Failed)           CTKVisualizationVTKWidgets
	271 - ctkVTKMagnifyViewTest2OddEven (Failed)            CTKVisualizationVTKWidgets
	272 - ctkVTKMagnifyViewTest2EvenOdd (Failed)            CTKVisualizationVTKWidgets
	278 - ctkVTKRenderViewEventTranslatorPlayerTest1 (Failed) CTKVisualizationVTKWidgets
	279 - ctkVTKScalarBarWidgetEventTranslatorPlayerTest1 (Failed) CTKVisualizationVTKWidgets
	280 - ctkVTKTextPropertyWidgetEventTranslatorPlayerTest1 (Timeout) CTKVisualizationVTKWidgets

Qt6 (10 failures):

	 38 - CTKPluginFrameworkAppTests (Subprocess aborted)   CTKPluginFramework
	 99 - ctkLanguageComboBoxTest (Failed)                  CTKWidgets
	147 - ctkWidgetsUtilsTestGrabWidget (Failed)            CTKWidgets
	148 - ctkWorkflowWidgetTest1 (Failed)                   CTKWidgets
	149 - ctkWorkflowWidgetTest2 (Failed)                   CTKWidgets
	206 - vtkLightBoxRendererManagerTest1 (SEGFAULT)        CTKVisualizationVTKCore
	249 - ctkVTKMagnifyViewTest2OddOdd (Failed)             CTKVisualizationVTKWidgets
	250 - ctkVTKMagnifyViewTest2EvenEven (Failed)           CTKVisualizationVTKWidgets
	251 - ctkVTKMagnifyViewTest2OddEven (Failed)            CTKVisualizationVTKWidgets
	252 - ctkVTKMagnifyViewTest2EvenOdd (Failed)            CTKVisualizationVTKWidgets

Generated by /validate-ctk-pr skill | cache: /home/johnsonhj/src/CTK/.claude/validate-cache

@hjmjohnson
hjmjohnson marked this pull request as ready for review April 3, 2026 13:41
@hjmjohnson
hjmjohnson force-pushed the fix-non-pod-constexpr-strings branch from 1e01702 to 3e92050 Compare April 3, 2026 13:41
@hjmjohnson

Copy link
Copy Markdown
Contributor Author

@jamesobutler I've been developing a method to more completely test each proposed change to CTK.

See the commit message: #1410 (comment).

I built the CTK source code

  • against Qt5
  • against Qt6
  • also used the same CTK code to test in a Slicer Build with Qt5
    to verify that no build or test regressions exist. This is the PR that I used to develop the validate-ctk-pr reports.

The report indicates no regressions to tests or builds.

If there are other bits of information you think would be helpful, let me know and I'll try to add them.

@jamesobutler
jamesobutler requested a review from lassoan April 3, 2026 16:44
@hjmjohnson
hjmjohnson force-pushed the fix-non-pod-constexpr-strings branch from 3e92050 to d67254c Compare April 13, 2026 19:28
@hjmjohnson
hjmjohnson force-pushed the fix-non-pod-constexpr-strings branch 2 times, most recently from 8aed437 to f9060b7 Compare April 27, 2026 12:19

@lassoan lassoan left a comment

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.

The proposed syntax and the repetitive utility function is somewhat noisy. According to Claude, there were also some potential ODR violations in ctkPluginFrameworkDebug_p.h.

I've submitted a pull request that describes an alternative solution (I could not push to this branch) - please have a look: #1440

@hjmjohnson

Copy link
Copy Markdown
Contributor Author

Closing in favor of #1440, which solves the same problem with a cleaner approach.

@lassoan's _L1 user-defined-literal backport (ctkStringLiterals.h) removes the repetitive helper this PR needed for the Qt5/Apple-clang constexpr case, and the static constexpr in-class members resolve the ODR concern he raised on this PR. #1440 covers the same six files, so nothing here is lost by closing it.

Thanks for the review and the alternative.

@hjmjohnson hjmjohnson closed this Aug 30, 2026
@lassoan

lassoan commented Aug 30, 2026

Copy link
Copy Markdown
Member

Thanks! Could you add a review to #1440 (to fulfill the review requirement)?

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.

2 participants