Skip to content

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

Open
lassoan wants to merge 1 commit into
commontk:masterfrom
lassoan:fix-non-pod-constexpr-strings
Open

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

Conversation

@lassoan

@lassoan lassoan commented Jul 18, 2026

Copy link
Copy Markdown
Member

This is a potential alternative solution to the same problem described in #1410, with a simpler syntax and without potential ODR violation issue.


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, now defined as constexpr QLatin1String. No call-site changes are required -- QLatin1String implicitly converts to QString for all existing uses.

Qt5 / Apple-clang compatibility: QLatin1String(const char*) calls strlen(), which is not constexpr under Apple clang. A new CTKCore header, ctkStringLiterals.h, provides a backport of Qt 6.4's Qt::StringLiterals::operator""_L1:

constexpr QLatin1String operator""_L1(const char* str, std::size_t size)
{ return QLatin1String{str, static_cast(size)}; }

The compiler supplies the literal length to the user-defined literal operator, so no strlen() is involved and construction is fully constexpr on all supported compilers. Definition sites are one-liners:

constexpr QLatin1String PREFIX_EXECUTABLE = "executable:"_L1;

The operator has external linkage and is shared by all translation units, so it is also safe to use in headers (an internal-linkage helper in an anonymous namespace would make in-class initializers an ODR violation). Once CTK requires Qt >= 6.4 the header can be retired in favor of Qt::StringLiterals with no call-site changes.

For ctkPluginFrameworkDebug, the ten OPTION_DEBUG_* class static members are converted to static constexpr in the private header using the literal and a #define CTK_OSGI prefix macro (scoped with #undef); string-literal concatenation is applied before the literal suffix, so CTK_OSGI "/debug"_L1 works. The out-of-line QString definitions and the CTK_OSGI QString variable are removed from the .cpp entirely (in-class static constexpr members are implicitly inline in C++17).

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

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, now defined as constexpr
QLatin1String. No call-site changes are required -- QLatin1String
implicitly converts to QString for all existing uses.

Qt5 / Apple-clang compatibility: QLatin1String(const char*) calls
strlen(), which is not constexpr under Apple clang. A new CTKCore
header, ctkStringLiterals.h, provides a backport of Qt 6.4's
Qt::StringLiterals::operator""_L1:

  constexpr QLatin1String operator""_L1(const char* str, std::size_t size)
  { return QLatin1String{str, static_cast<int>(size)}; }

The compiler supplies the literal length to the user-defined literal
operator, so no strlen() is involved and construction is fully
constexpr on all supported compilers. Definition sites are one-liners:

  constexpr QLatin1String PREFIX_EXECUTABLE = "executable:"_L1;

The operator has external linkage and is shared by all translation
units, so it is also safe to use in headers (an internal-linkage helper
in an anonymous namespace would make in-class initializers an ODR
violation). Once CTK requires Qt >= 6.4 the header can be retired in
favor of Qt::StringLiterals with no call-site changes.

For ctkPluginFrameworkDebug, the ten OPTION_DEBUG_* class static
members are converted to static constexpr in the private header using
the literal and a #define CTK_OSGI prefix macro (scoped with #undef);
string-literal concatenation is applied before the literal suffix, so
CTK_OSGI "/debug"_L1 works. The out-of-line QString definitions and
the CTK_OSGI QString variable are removed from the .cpp entirely
(in-class static constexpr members are implicitly inline in C++17).

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

Co-authored-by: Hans J. Johnson <hans-johnson@uiowa.edu>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lassoan
lassoan force-pushed the fix-non-pod-constexpr-strings branch from b9fd9c9 to dbee151 Compare August 30, 2026 16:23
@hjmjohnson

Copy link
Copy Markdown
Contributor

Reviewed and build-tested this locally against upstream/master (868d56c7). Builds clean on Qt5 and Qt6 with no new warnings, and I found no functional defect. Two small hardening suggestions below, both about the new public header rather than the conversion itself.

Worth flagging first: CI does not compile any of the six files this PR touches. CTK_LIB_PluginFramework defaults to OFF (CMakeLists.txt:497) and the workflow never enables it; CTK_LIB_CommandLineModules/Frontend/QtGui also defaults OFF and is forced OFF under Qt6 (CMakeLists.txt:552). The green checks here are real but they are not evidence about this change, so I enabled those modules locally to get actual coverage.

Build and test evidence (Qt5 + Qt6, proposed vs upstream/master)

Configuration: CTK_LIB_PluginFramework=ON, CTK_LIB_CommandLineModules/Core=ON, CTK_LIB_CommandLineModules/Frontend/QtGui=ON.

Config Target(s) 868d56c7 baseline b9fd9c94 proposed
Qt5 5.15.13 PluginFramework + CmdLineModules/Frontend/QtGui 0 errors / 0 warnings 0 errors / 0 warnings
Qt6 PluginFramework 0 errors / 0 warnings 0 errors / 0 warnings

All five changed .cpp files were confirmed recompiled on both sides (the sixth changed file is ctkPluginFrameworkDebug_p.h). The only warning seen in either log is -Wdeprecated-declarations on QtConcurrent::Exception in ctkCmdLineModuleRunException.cpp, a file this PR does not touch.

Tests, same configuration, both refs: CTKPluginFrameworkTests and CTKPluginFrameworkTests.perf pass. CTKPluginFrameworkAppTests aborts identically on upstream/master, so it is pre-existing and not a regression.

Call sites were checked individually: every converted constant reaches either a const QString& parameter or a QLatin1String overload, and no converted constant is used to construct a QVariant. INSTALL_HASH_PLACEHOLDER.size() changes from UTF-16 units to Latin-1 bytes, which is identical for ASCII. Behavior is preserved.

Why the downstream Slicer axis could not be exercised

I normally validate CTK changes through a 3D Slicer superbuild as well. That is not informative for this PR, for a structural reason: Slicer sets CTK_LIB_PluginFramework:BOOL=OFF in SuperBuild/External_CTK.cmake:119, so a Slicer build cannot compile any of the files changed here regardless of configuration. The most it could have shown is that the newly installed ctkStringLiterals.h does not disturb CTK's export/install.

For completeness: my local Slicer superbuild tree also failed to configure on both refs (a stale path baked into its cache, unrelated to this PR), so even that limited signal is absent. I did not want to imply downstream coverage that does not exist.

Suggestion 1 — version-guard operator""_L1

Libs/Core/ctkStringLiterals.h defines operator""_L1 unconditionally, and Libs/Core/CMakeLists.txt:84 installs the header. On Qt >= 6.4 any translation unit that has both Qt::StringLiterals and ctk::string_literals in scope gets an ambiguous user-defined literal, which is a hard error. Nothing in CTK or Slicer uses _L1 today, so this is latent rather than broken, but it becomes reachable for downstream consumers the moment they adopt Qt::StringLiterals.

#if QT_VERSION < QT_VERSION_CHECK(6, 4, 0)
constexpr QLatin1String operator""_L1(const char* str, std::size_t size)
{
  return QLatin1String{str, static_cast<int>(size)};
}
#endif
Suggestion 2 — scope the using-directive in the private header

Libs/PluginFramework/ctkPluginFrameworkDebug_p.h:34 places the directive at global scope:

using namespace ctk::string_literals;

The other four call sites scope it inside a namespace block. This matters more than usual because CMake/ctkMacroBuildLib.cmake:164 installs headers via file(GLOB "*.h"), which includes _p.h, so the directive is exported to any consumer that includes it. Because string_literals is an inline namespace, a plain using namespace ctk; also pulls _L1 in. Combined with suggestion 1 this is what makes the ambiguity reachable from outside CTK.

Minor notes (no action needed)
  • The PR description says "No call-site changes are required", but ctkLocationManager.cpp:144 and :286 did need rewrapping, since const char* + QLatin1String has no overload. STYLE: Replace non-POD string constants with constexpr QLatin1String #1410 needed the identical two edits, so this is a description nit rather than a defect.
  • QString(".") + CTK at ctkLocationManager.cpp:286 allocates a QString for a one-character literal; '.' + CTK would avoid it.
  • #define CTK_OSGI / #undef CTK_OSGI is correctly balanced, though it is an unnamespaced macro in an installed header.
  • _L1 is technically a reserved identifier (underscore + uppercase), but matching Qt's spelling is the entire point of the backport, so this seems right as-is.
  • The C++17 requirement (CMakeLists.txt:57-64) makes the in-class static constexpr members implicitly inline, so removing the ten out-of-line definitions is correct and the ODR concern raised on STYLE: Replace non-POD string constants with constexpr QLatin1String #1410 is genuinely resolved here.

I closed #1410 in favor of this PR.

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