Skip to content

Conversation

@wdconinc
Copy link
Contributor

This PR changes header-exposed TU-local entities (constexpr variables and variable templates) from static to inline. This allows them to be used in C++20 modules (#907).

BEGINRELEASENOTES

  • Move static constexpr to inline constexpr to avoid exposed TU-local entities

ENDRELEASENOTES

Copilot AI review requested due to automatic review settings December 22, 2025 17:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR changes header-exposed TU-local entities (constexpr variables and variable templates) from static constexpr to inline constexpr to enable their use in C++20 modules. The changes address the issue that static constexpr variables create translation-unit-local entities that cannot be properly exposed across module boundaries.

  • Converted variable templates and constexpr variables from static constexpr to inline constexpr across multiple header files
  • Updated tag variables and string_view constants to use inline storage linkage
  • Maintained consistent keyword ordering (inline constexpr) throughout most of the codebase

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
include/podio/utilities/TypeHelpers.h Converted 8 variable templates from static constexpr to inline constexpr for type trait helpers
include/podio/utilities/MaybeSharedPtr.h Changed MarkOwned tag variable from constexpr static to inline constexpr
include/podio/detail/LinkFwd.h Updated 4 string_view constants for link naming from implicit static to inline linkage
include/podio/detail/Link.h Modified 2 variable templates in LinkT class, though with inconsistent keyword ordering
include/podio/LinkNavigator.h Changed ReturnFrom and ReturnTo tag variables to inline constexpr
include/podio/GenericParameters.h Updated isSupportedGenericDataType variable template to inline constexpr

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tmadlener
Copy link
Collaborator

Should the following also become inline constexpr static?

constexpr static std::string_view typeName = "{{ (class | string ).strip(':') + "Collection" }}";
constexpr static std::string_view valueTypeName = "{{ (class | string ).strip(':') }}";
constexpr static std::string_view dataTypeName = "{{ ( class | string ).strip(':') }}Data";

And then similar here

constexpr static std::string_view typeName =
podio::utils::static_concatenate_v<podio::detail::link_coll_name_prefix, FromT::typeName,
podio::detail::link_name_infix, ToT::typeName,
podio::detail::link_name_suffix>;
constexpr static std::string_view valueTypeName = value_type::typeName;
constexpr static std::string_view dataTypeName = "podio::LinkData";

and here

static constexpr std::string_view typeName = userDataCollTypeName<BasicType>();
static constexpr std::string_view valueTypeName = userDataTypeName<BasicType>();
static constexpr std::string_view dataTypeName = userDataTypeName<BasicType>();

@wdconinc
Copy link
Contributor Author

Should the following also become inline constexpr static?

Hmm... Those have not caused any issues in building modules (I'll admit this was mostly solved using whack-a-mole strategy). I think the difference is that they are not templates, and therefore they can have external linkage.

@tmadlener
Copy link
Collaborator

OK. Then maybe leave them as is for the moment. They are also connected to ROOT startup times, see #808 and #775 (resp. we have put in some mitigations for trying to reduce startup time). I am not entirely sure how the whole interaction would be with all of that.

@tmadlener tmadlener force-pushed the static-in-headers-to-inline branch from 0d7d340 to 561536a Compare December 23, 2025 09:14
@tmadlener tmadlener enabled auto-merge (squash) December 23, 2025 09:17
@tmadlener tmadlener merged commit 5d82748 into AIDASoft:master Dec 23, 2025
26 checks passed
@wdconinc wdconinc deleted the static-in-headers-to-inline branch December 23, 2025 15:35
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