Skip to content

Conversation

@eddyashton
Copy link
Member

We previously had this comment, but it's easy to miss:

    // Must contain a static function with signature:
    // static char const* get_subsystem_name()

And then you'd get an error at the point of use, if your new type didn't meet this requirement:

  struct NotASubsystem { };

  auto nope = std::make_shared<NotASubsystem>();
  install_subsystem(nope); // NB: This is node_stub.h:283
In file included from /workspaces/CCF/src/node/rpc/test/node_frontend_test.cpp:8:
In file included from /workspaces/CCF/src/node/rpc/test/frontend_test_infra.h:7:
In file included from /workspaces/CCF/include/ccf/app_interface.h:5:
In file included from /workspaces/CCF/include/ccf/common_endpoint_registry.h:5:
In file included from /workspaces/CCF/include/ccf/base_endpoint_registry.h:9:
/workspaces/CCF/include/ccf/node_context.h:56:39: error: no member named 'get_subsystem_name' in 'ccf::NotASubsystem'
   56 |       install_subsystem(subsystem, T::get_subsystem_name());
      |                                       ^
/workspaces/CCF/src/node/rpc/test/node_stub.h:283:7: note: in instantiation of function template specialization 'ccf::AbstractNodeContext::install_subsystem<ccf::NotASubsystem>' requested here
  283 |       install_subsystem(nope);

This kind of error is familiar to seasoned C++ developers, but ugly - it is thrown during template substitution, dumping the include-stack of a bunch of internal headers but not including the line in our new code that actually triggers this. Fundamentally, the fact that install_subsystem requires get_subsystem_name() is only discovered when you hit this error, or by noticing the comment, and working out the required return type requires further spelunking or error message iteration.

By writing this requirement as an explicit constraint we get an earlier, clearer (and more verbose, for better and worse) error:

In file included from /workspaces/CCF/src/node/rpc/test/node_frontend_test.cpp:8:
In file included from /workspaces/CCF/src/node/rpc/test/frontend_test_infra.h:17:
/workspaces/CCF/src/node/rpc/test/node_stub.h:283:7: error: no matching member function for call to 'install_subsystem'
  283 |       install_subsystem(nope);
      |       ^~~~~~~~~~~~~~~~~
/workspaces/CCF/include/ccf/node_context.h:54:10: note: candidate template ignored: constraints not satisfied [with T = ccf::NotASubsystem]
   54 |     void install_subsystem(const std::shared_ptr<T>& subsystem)
      |          ^
/workspaces/CCF/include/ccf/node_context.h:53:15: note: because 'ccf::NotASubsystem' does not satisfy 'SubsystemType'
   53 |     template <SubsystemType T>
      |               ^
/workspaces/CCF/include/ccf/node_subsystem_interface.h:15:10: note: because 'T::get_subsystem_name()' would be invalid: no member named 'get_subsystem_name' in 'ccf::NotASubsystem'
   15 |     { T::get_subsystem_name() } -> std::convertible_to<std::string_view>;

Including the correct line in the callstack is a huge win, and better yet we don't need to look at how install_subsystem() is actually implemented - its interface/signature precisely describes the requirement, and is all we need to look at to produce write a fix.

Those are the pros, but let's just list some cons for discussion:

  • This is the first concept in our code. Creeping complexity of adopting new language features.
  • The syntax of requirements is unusual, and currently unfamiliar. Can you realistically parse { T::get_subsystem_name() } -> std::convertible_to<std::string_view> as "your type must have a static function named get_subsystem_name() returning something convertible to a string"? With squinting, and practice.
  • This is still an error at point-of-use rather than point-of-definition. I'd really like to say that my new type implements/inherits-from this concept, and be told I didn't do that where the type is defined, rather than where its used. But afaict this isn't possible? Lmk if your google-fu disagrees ("C++ concept"? Ungoogleable, disastrous naming).

@eddyashton eddyashton requested a review from a team as a code owner January 14, 2026 11:49
Copilot AI review requested due to automatic review settings January 14, 2026 11:49
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 introduces a C++20 concept to enforce at compile-time that subsystem types provide a get_subsystem_name() static method. Previously, this requirement was only documented in a comment, leading to confusing template substitution errors when violated.

Changes:

  • Defined SubsystemType concept requiring get_subsystem_name() returning a string-like type and inheritance from AbstractNodeSubSystem
  • Applied the concept constraint to template parameters in install_subsystem() and get_subsystem() methods
  • Renamed internal helper methods to *_by_name to clarify distinction from the public template methods

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
include/ccf/node_subsystem_interface.h Defines the new SubsystemType concept with requirements for subsystem types
include/ccf/node_context.h Applies the concept constraint to template methods and renames internal helpers for clarity

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