feat: add plugin support#378
Open
Daan Timmer (daantimmer) wants to merge 29 commits into
Open
Conversation
✅
|
| Descriptor | Linter | Files | Fixed | Errors | Warnings | Elapsed time |
|---|---|---|---|---|---|---|
| ✅ ACTION | actionlint | 6 | 0 | 0 | 0.27s | |
| ✅ ACTION | zizmor | 6 | 0 | 0 | 0 | 1.55s |
| ✅ CPP | clang-format | 257 | 1 | 0 | 0 | 1.66s |
| ✅ DOCKERFILE | hadolint | 1 | 0 | 0 | 0.04s | |
| markdownlint | 8 | 5 | 20 | 0 | 1.09s | |
| ✅ MARKDOWN | markdown-table-formatter | 8 | 5 | 0 | 0 | 0.29s |
| ✅ REPOSITORY | betterleaks | yes | no | no | 1.04s | |
| ✅ REPOSITORY | git_diff | yes | no | no | 0.03s | |
| ✅ REPOSITORY | grype | yes | no | no | 56.63s | |
| ✅ REPOSITORY | ls-lint | yes | no | no | 0.01s | |
| ✅ REPOSITORY | secretlint | yes | no | no | 4.85s | |
| ✅ REPOSITORY | syft | yes | no | no | 2.07s | |
| ✅ REPOSITORY | trivy | yes | no | no | 10.69s | |
| ✅ REPOSITORY | trivy-sbom | yes | no | no | 0.29s | |
| ✅ REPOSITORY | trufflehog | yes | no | no | 4.86s | |
| lychee | 76 | 1 | 0 | 9.8s | ||
| ✅ YAML | prettier | 13 | 1 | 0 | 0 | 0.62s |
| ✅ YAML | v8r | 13 | 0 | 0 | 8.14s | |
| ✅ YAML | yamllint | 13 | 0 | 0 | 0.65s |
Detailed Issues
⚠️ SPELL / lychee - 1 error
📝 Summary
---------------------
🔍 Total..........168
🔗 Unique.........162
✅ Successful.....167
⏳ Timeouts.........0
🔀 Redirected......74
👻 Excluded.........0
❓ Unknown..........0
🚫 Errors...........1
⛔ Unsupported......1
Errors in CONTRIBUTING.md
[404] https://github.com/yourname/amp-cucumber-cpp-runner.git (at 41:36) | Rejected status code: 404 Not Found | Followed 1 redirect. Redirects: https://github.com/yourname/amp-cucumber-cpp-runner.git --[301]--> https://github.com/yourname/amp-cucumber-cpp-runner
Hint: Followed 74 redirects. You might want to consider replacing redirecting URLs with the resolved URLs. Use verbose mode (`-v`/`-vv`) to see redirection details.
Hint: You can configure accepted/rejected response codes with `-a` or `--accept`
⚠️ MARKDOWN / markdownlint - 20 errors
CHANGELOG.md:31 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:38 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Bug Fixes"]
CHANGELOG.md:44 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:51 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:58 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:66 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "⚠ BREAKING CHANGES"]
CHANGELOG.md:70 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:79 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:85 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:93 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "⚠ BREAKING CHANGES"]
CHANGELOG.md:97 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:102 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Bug Fixes"]
CHANGELOG.md:111 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:122 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:130 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "⚠ BREAKING CHANGES"]
CHANGELOG.md:134 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:159 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Bug Fixes"]
CHANGELOG.md:166 error MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
cmake/WindowsToolchainFilesProvidedBy.md:1 error MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "<https://github.com/MarkSchofi..."]
README.md:132 error MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
Notices
📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining SECURITY_SUGGESTIONS: false)
See detailed reports in MegaLinter artifacts
Your project could benefit from a custom flavor, which would allow you to run only the linters you need, and thus improve runtime performances. (Skip this info by defining FLAVOR_SUGGESTIONS: false)
- Documentation: Custom Flavors
- Command:
npx mega-linter-runner@9.6.0 --custom-flavor-setup --custom-flavor-linters ACTION_ACTIONLINT,ACTION_ZIZMOR,CPP_CLANG_FORMAT,DOCKERFILE_HADOLINT,MARKDOWN_MARKDOWNLINT,MARKDOWN_MARKDOWN_TABLE_FORMATTER,REPOSITORY_GIT_DIFF,REPOSITORY_BETTERLEAKS,REPOSITORY_GRYPE,REPOSITORY_LS_LINT,REPOSITORY_SECRETLINT,REPOSITORY_SYFT,REPOSITORY_TRIVY,REPOSITORY_TRIVY_SBOM,REPOSITORY_TRUFFLEHOG,SPELL_LYCHEE,YAML_PRETTIER,YAML_YAMLLINT,YAML_V8R
Show us your support by starring ⭐ the repository
Contributor
There was a problem hiding this comment.
Pull request overview
Adds runtime plugin support to the cucumber-cpp runner/library, enabling step/hook/parameter definitions to be loaded from dynamic libraries via a new --load CLI option. This extends the existing registration model to support external modules and updates examples/compatibility coverage accordingly.
Changes:
- Introduces a plugin subsystem (
DynamicLibrary,DynamicLibraryManager, loader helpers, and CMakeccr_add_plugin) and wires--loadintoApplication. - Adjusts parameter/step/hook loading order to ensure parameters are available before step compilation.
- Updates examples and the compatibility test harness to build/load plugins at runtime.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cucumber_cpp/runner/CMakeLists.txt | Documents ENABLE_EXPORTS requirement for executables hosting plugins. |
| cucumber_cpp/library/support/DefinitionRegistration.hpp | Adds Clear() API for resetting global registrations. |
| cucumber_cpp/library/support/DefinitionRegistration.cpp | Implements DefinitionRegistration::Clear(). |
| cucumber_cpp/library/plugin/StepLoader.hpp | Adds step-registry loader wrapper for plugin workflow. |
| cucumber_cpp/library/plugin/StepLoader.cpp | Implements StepLoader::Load() calling StepRegistry::LoadSteps(). |
| cucumber_cpp/library/plugin/PluginRegister.cpp | Provides default exported ccr_register entry point for plugins. |
| cucumber_cpp/library/plugin/PluginExport.hpp | Defines export macro and register-function type for plugin entry point. |
| cucumber_cpp/library/plugin/ParameterLoader.hpp | Adds loader for dynamically registered parameter types into ParameterRegistry. |
| cucumber_cpp/library/plugin/ParameterLoader.cpp | Implements parameter loading from DefinitionRegistration into ParameterRegistry. |
| cucumber_cpp/library/plugin/HookLoader.hpp | Adds hook-registry loader wrapper for plugin workflow. |
| cucumber_cpp/library/plugin/HookLoader.cpp | Implements HookLoader::Load() calling HookRegistry::LoadHooks(). |
| cucumber_cpp/library/plugin/DynamicLibraryManager.hpp | Declares manager for loading/unloading libraries and listing loaded modules. |
| cucumber_cpp/library/plugin/DynamicLibraryManager.cpp | Implements path-based loading (file/dir) and invokes ccr_register. |
| cucumber_cpp/library/plugin/DynamicLibrary.hpp | Declares cross-platform dynamic library wrapper and symbol lookup. |
| cucumber_cpp/library/plugin/DynamicLibrary.cpp | Implements dlopen/LoadLibrary, symbol resolution, and extension selection. |
| cucumber_cpp/library/plugin/CMakeLists.txt | Adds new cucumber_cpp.library.plugin target and links required libs. |
| cucumber_cpp/library/cucumber_expression/ParameterRegistry.hpp | Adds converter map clearer mechanism and adjusts API visibility. |
| cucumber_cpp/library/CMakeLists.txt | Adds plugin library to build and links it into the main library target. |
| cucumber_cpp/library/Application.hpp | Adds --load option storage and a DynamicLibraryManager member + destructor. |
| cucumber_cpp/library/Application.cpp | Implements --load handling and teardown logic for plugin lifecycle. |
| cucumber_cpp/library/api/RunCucumber.cpp | Switches to phased loading: parameters → steps → hooks via plugin loaders. |
| cucumber_cpp/example/plugin/PluginSteps.cpp | Adds example plugin step definitions. |
| cucumber_cpp/example/plugin/PluginHooks.cpp | Adds example plugin hooks. |
| cucumber_cpp/example/plugin/features/plugin.feature | Adds feature demonstrating plugin-provided steps. |
| cucumber_cpp/example/plugin/CMakeLists.txt | Builds the example plugin via ccr_add_plugin. |
| cucumber_cpp/example/CMakeLists.txt | Enables exports for example executable and adds plugin example subdir. |
| cucumber_cpp/CucumberCpp.hpp | Exposes plugin export header publicly. |
| compatibility/TestCompatibility.cpp | Converts compatibility tests to parameterized suite and plugin discovery/loading. |
| compatibility/retry-undefined/retry-undefined.cpp | Adds empty kit source for plugin build/discovery logic. |
| compatibility/hooks-attachment/hooks-attachment.cpp | Uses source_location to derive kit directory at runtime. |
| compatibility/examples-tables-attachment/examples-tables-attachment.cpp | Uses source_location to derive kit directory at runtime. |
| compatibility/empty/empty.cpp | Adds empty kit source for plugin build/discovery logic. |
| compatibility/CMakeLists.txt | Builds per-kit plugins and a single test executable that loads plugins at runtime. |
| compatibility/BaseCompatibility.hpp | Replaces Devkit with KitInfo and updates RunDevkit signature. |
| compatibility/BaseCompatibility.cpp | Reworks devkit runner to invoke Application and compare NDJSON outputs. |
| compatibility/attachments/attachments.cpp | Uses source_location to derive kit directory at runtime. |
| cmake/ccr_add_plugin.cmake | Adds helper for building MODULE plugins with the default ccr_register. |
| .vscode/settings.json | Updates step-glob for cucumber autocomplete to include new example layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
cucumber_cpp/library/Application.cpp:83
- In the destructor, the plugins are unloaded (dlclose/FreeLibrary) before clearing DefinitionRegistration and converter type maps. Those containers can hold std::function/closures whose destructors may reside in the plugin; destroying them after unload is undefined behavior and can crash. Clear registries/type maps first, then unload libraries.
if (!dynamicLibraryManager.GetLoadedLibraries().empty())
{
dynamicLibraryManager.UnloadAll();
support::DefinitionRegistration::Instance().Clear();
cucumber_expression::ConverterTypeMapClearer::ClearAll();
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
cucumber_cpp/library/support/DefinitionRegistration.cpp:70
LoadIds()now assigns IDs by iteratingstaticRegistryand thenregistry, which changes the overall ID assignment order compared to ordering bySourceLocationOrder(file+line). If IDs are expected to be deterministic across static+plugin registrations, this should merge-iterate both maps by key so the generator is applied in global source-location order.
cucumber_cpp/library/support/DefinitionRegistration.cpp:86GetHooks()collects hooks fromstaticRegistryandregistryin two separate passes, which breaks the existing global ordering implied bySourceLocationOrder(file+line). If hook execution/emit order is meant to be stable across static + plugin definitions, iterate both maps in merged key order.
cucumber_cpp/library/support/DefinitionRegistration.hpp:95ForEachRegisteredStep()iteratesstaticRegistryandregistryseparately, which changes the global step ordering fromSourceLocationOrder(file+line) to a grouped "static then dynamic" order. If consumers rely on deterministic ordering, consider merge-iterating both maps by key.
cucumber_cpp/library/plugin/DynamicLibrary.cpp:31- On Windows,
FormatMessageA()can return 0 and leavebuffer == nullptr. Constructingstd::string message(buffer, size)would then be undefined behavior. Add a failure check before usingbuffer, and onlyLocalFree()when a buffer was actually allocated.
LPSTR buffer = nullptr;
const auto size = FormatMessageA(
FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
nullptr, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
reinterpret_cast<LPSTR>(&buffer), 0, nullptr); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
compatibility/CMakeLists.txt:53
- This uses
target_link_options()to pass library file paths ($<TARGET_FILE:...>) between--whole-archive/--no-whole-archive. CMake treats these as options, not link items, and this is brittle across generators / multi-config builds and can misbehave when the targets are not static archives. Prefer expressing whole-archive intarget_link_libraries()(e.g.$<LINK_LIBRARY:WHOLE_ARCHIVE,lib>with CMake 3.24+) and keeptarget_link_options()for flags only.
"LINKER:--whole-archive"
$<TARGET_FILE:cucumber_cpp.library>
$<TARGET_FILE:cucumber_cpp.library.api>
$<TARGET_FILE:cucumber_cpp.library.engine>
$<TARGET_FILE:cucumber_cpp.library.support>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cucumber_cpp/library/Application.cpp:83
- In the destructor, plugins are unloaded before clearing
DefinitionRegistration. Since plugin-loaded steps/parameters can registerstd::function/lambdas whose manager/invoker code lives in the plugin module, destroying them afterdlclose/FreeLibraryis undefined behavior and can crash. Clear the dynamic registrations before unloading the libraries.
Application::~Application()
{
if (!dynamicLibraryManager.GetLoadedLibraries().empty())
{
dynamicLibraryManager.UnloadAll();
support::DefinitionRegistration::Instance().Clear();
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
cucumber_cpp/library/support/DefinitionRegistration.cpp:41
RegisterPluginappends the plugin pointer unconditionally (except for self). Ifccr_registeris called more than once for the same DLL (or the loader retries), this will duplicate the same plugin inpluginsand cause duplicate iteration/ID assignment.
cucumber_cpp/library/plugin/DynamicLibraryManager.cpp:50LoadFilestores the library inlibrariesbefore validating thatccr_registerexists and can be invoked. If symbol lookup/registration throws, the manager retains a loaded library even thoughLoad()failed (until an explicitUnloadAll()), which can make error recovery unpredictable.
void DynamicLibraryManager::LoadFile(const std::filesystem::path& path)
{
const auto& lib = libraries.emplace_back(path);
auto registerFn = lib.GetSymbol<CcrRegisterFn>("ccr_register");
registerFn(&support::DefinitionRegistration::Instance());
}
cucumber_cpp/library/plugin/DynamicLibrary.cpp:122
- On POSIX,
dlsym()error detection should usedlerror()rather than checking fornullptronly. Without clearing/checkingdlerror(), you can mis-report failures (and you lose the underlying loader error message).
void* DynamicLibrary::GetSymbolAddress(std::string_view name) const
{
const std::string symbolName{ name };
#if defined(_WIN32)
void* symbol = reinterpret_cast<void*>(GetProcAddress(static_cast<HMODULE>(handle), symbolName.c_str())); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
#else
void* symbol = dlsym(handle, symbolName.c_str());
#endif
if (symbol == nullptr)
throw std::runtime_error("Symbol '" + symbolName + "' not found in library '" + libraryPath.string() + "'");
return symbol;
cucumber_cpp/library/Application.cpp:87
- Clearing
ConverterTypeMapClearer::ClearFunctions()breaks subsequent plugin loads/unloads:ConverterTypeMap<T>::Instance()uses astatic const bool registeredguard, so after the first.clear()the per-type clear callbacks are never re-registered. This can leave converter maps uncleared on later unloads (risking dangling function pointers into unloaded plugins).
if (!dynamicLibraryManager.GetLoadedLibraries().empty())
{
cucumber_expression::ConverterTypeMapClearer::ClearAll();
cucumber_expression::ConverterTypeMapClearer::ClearFunctions().clear();
support::DefinitionRegistration::Instance().UnregisterPlugins();
dynamicLibraryManager.UnloadAll();
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
cucumber_cpp/library/Application.cpp:87
- Clearing ConverterTypeMapClearer::ClearFunctions() breaks subsequent runs in the same process: the per-template
registeredflag in ConverterTypeMap::Instance() prevents re-registering clear callbacks, so future plugin unloads won’t clear converter maps (potentially leaving stale converters pointing into unloaded plugins).
if (!dynamicLibraryManager.GetLoadedLibraries().empty())
{
cucumber_expression::ConverterTypeMapClearer::ClearAll();
cucumber_expression::ConverterTypeMapClearer::ClearFunctions().clear();
support::DefinitionRegistration::Instance().UnregisterPlugins();
dynamicLibraryManager.UnloadAll();
cucumber_cpp/library/util/Duration.hpp:19
- Stopwatch stores a global
instancepointer set in the constructor, but the (now virtual) destructor doesn’t clear it. If the current instance is destroyed (e.g., Application teardown),Stopwatch::Instance()can later dereference a dangling pointer.
cucumber_cpp/library/support/DefinitionRegistration.cpp:41 - RegisterPlugin() can register the same plugin instance multiple times (e.g., duplicate --load paths). That would cause repeated iteration over the same registry and duplicate steps/hooks/parameters in outputs. Consider de-duplicating the plugins list.
cucumber_cpp/library/plugin/DynamicLibrary.cpp:35 - On Windows, FormatMessageA can return size==0 and leave
buffer == nullptr. Constructingstd::string message(buffer, size)in that case is undefined behavior. Guard against FormatMessage failure before building the string.
LPSTR buffer = nullptr;
const auto size = FormatMessageA(
FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
nullptr, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
reinterpret_cast<LPSTR>(&buffer), 0, nullptr); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
std::string message(buffer, size);
LocalFree(buffer);
return message;
cucumber_cpp/library/plugin/DynamicLibraryManager.cpp:50
- If GetSymbol() or ccr_register() throws after emplace_back(), the library stays in
libraries(and the handle remains open) even though registration failed. Roll back the push on failure to avoid leaving partially-loaded plugins around.
void DynamicLibraryManager::LoadFile(const std::filesystem::path& path)
{
const auto& lib = libraries.emplace_back(path);
auto registerFn = lib.GetSymbol<CcrRegisterFn>("ccr_register");
cucumber_cpp/library/plugin/DynamicLibrary.cpp:120
- On POSIX, checking
dlsym(...) == nullptris not a reliable error check (a symbol’s address can be null). The recommended pattern is to cleardlerror()before dlsym and checkdlerror()after; only treat it as failure when dlerror reports an error.
void* symbol = dlsym(handle, symbolName.c_str());
#endif
if (symbol == nullptr)
throw std::runtime_error("Symbol '" + symbolName + "' not found in library '" + libraryPath.string() + "'");
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
cucumber_cpp/library/plugin/DynamicLibraryManager.cpp:51
LoadFile()emplaces the library intolibrariesbefore validating thatccr_registerexists and before calling it. IfGetSymbol()or the registration function throws, the manager will still consider the library loaded (and keep it inlibraries), which can lead to partially-initialized state and misleading results fromGetLoadedLibraries().
Consider loading into a temporary DynamicLibrary and only moving it into libraries after successful registration (so failures leave no residue).
void DynamicLibraryManager::LoadFile(const std::filesystem::path& path)
{
const auto& lib = libraries.emplace_back(path);
auto registerFn = lib.GetSymbol<CcrRegisterFn>("ccr_register");
cucumber_cpp/library/support/DefinitionRegistration.cpp:39
RegisterPlugin()appends blindly toplugins. If the same plugin is registered more than once (e.g., duplicate--loadpaths or repeatedccr_registercalls), steps/hooks/parameters can be enumerated multiple times and IDs can be assigned repeatedly.
Add a simple duplicate guard before pushing the pointer.
cucumber_cpp/library/util/Duration.hpp:19
Stopwatchupdates the globalinstancepointer in its constructor, but the destructor does not clear/reset it. With the newSetInstance()and injectedStopwatchlifetimes (e.g.,Applicationowns aunique_ptr<Stopwatch>), destroying the active stopwatch can leaveStopwatch::instancedangling and cause UB on the nextStopwatch::Instance()call.
Clear the singleton pointer when the active instance is destroyed.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
cucumber_cpp/library/plugin/DynamicLibraryManager.cpp:51
LoadFile()adds the library tolibrariesbefore validating theccr_registersymbol and invoking it. IfGetSymbolor the plugin'sccr_registerthrows, the manager will retain a partially-initialized loaded library in its state (and potentially keep it loaded) even thoughLoad()failed. Consider constructing theDynamicLibraryon the stack first and only moving it intolibrariesafter successful registration.
void DynamicLibraryManager::LoadFile(const std::filesystem::path& path)
{
const auto& lib = libraries.emplace_back(path);
auto registerFn = lib.GetSymbol<CcrRegisterFn>("ccr_register");
cucumber_cpp/library/support/DefinitionRegistration.cpp:40
RegisterPlugin()unconditionallypush_backs the plugin pointer (except self-guard). If the same plugin is registered twice (e.g., repeated--load, duplicates in a directory, or re-entrant calls), lookups will iterate it multiple times leading to duplicated steps/hooks/parameters and potentially duplicated ID assignment. Consider de-duplicating before insertion.
…y migration on Linux platforms
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
cucumber_cpp/library/util/Duration.hpp:23
Stopwatchstores a raw global singleton pointer (instance) but its destructor is defaulted and never clears the pointer. When the owningApplication(or a test) destroys itsStopwatchimplementation,Stopwatch::Instance()can later return a dangling reference.
Clear instance in the destructor when it points at this.
cucumber_cpp/library/support/DefinitionRegistration.cpp:41
RegisterPlugin()unconditionally appends the plugin pointer. If a plugin is loaded twice (duplicate--loadpaths, or the same DLL loaded more than once), the sameDefinitionRegistration*can be stored multiple times, causing duplicated steps/hooks/parameters during iteration.
Deduplicate by pointer before inserting.
Daan Timmer (daantimmer)
changed the title
- Introduced PluginHostContext struct with ABI version and size. - Implemented snapshot and restore functionality for converter registry. - Updated plugin registration to check ABI version for compatibility. - Enhanced dynamic library management with improved error reporting. - Refactored CMake configuration for plugin linking. - Added build directory handling in compatibility checks.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
cucumber_cpp/library/util/Body.hpp:7
Body.hppincludesgtest/gtest.heven though it only forward-declaresCucumberResultReporterand doesn't use gtest types in the header. This unnecessarily couples a public header to gtest and increases compile cost (the .cpp already includes gtest).
Comment on lines
+94
to
+101
| plugin::DynamicLibraryManager dynamicLibraryManager; | ||
|
|
||
| std::unique_ptr<PluginSession> pluginSession; | ||
|
|
||
| cucumber_expression::ParameterRegistry parameterRegistry{ cucumber_cpp::library::support::DefinitionRegistration::Instance().GetRegisteredParameters() }; | ||
| bool removeDefaultGoogleTestListener; | ||
| util::StopWatchHighResolutionClock stopwatchHighResolutionClock; | ||
| util::TimestampGeneratorSystemClock timestampGeneratorSystemClock; | ||
| std::unique_ptr<util::Stopwatch> stopwatch; | ||
| std::unique_ptr<util::TimestampGenerator> timestampGenerator; |
Comment on lines
+9
to
+14
| if (context == nullptr || context->registration == nullptr) | ||
| return; | ||
|
|
||
| if (context->abiVersion != cucumber_cpp::library::plugin::pluginAbiVersion) | ||
| return; | ||
|
|
Comment on lines
+367
to
+375
| struct PluginHostContext | ||
| { | ||
| uint32_t abi_version; // pluginAbiVersion constant | ||
| uint32_t struct_size; // sizeof(PluginHostContext) at build time | ||
| void* registration; | ||
| void* stopwatch; | ||
| void* timestampGenerator; | ||
| void* converterMap; | ||
| }; |
Comment on lines
+43
to
+46
| support::DefinitionRegistration::Instance().TakeSnapshot(); | ||
| cucumber_expression::ConverterRegistry::TakeSnapshot(); | ||
| manager.Load(paths); | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 64 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
cucumber_cpp/library/util/Body.hpp:7
gtest/gtest.his included in this public header but no symbols from it are used here. This unnecessarily leaks the gtest dependency (and increases compile time) to any translation unit includingutil/Body.hpp;Body.cppalready includes the needed gtest headers for the reporter implementation.
cucumber_cpp/library/plugin/PluginRegister.cpp:14PluginHostContextcontainsstructSize, butccr_registernever validates it before reading later fields. If a newer plugin is loaded by an older host (sameabiVersionbut smaller struct), readingcontext->stopwatch/converterMapis undefined behavior. Add a size check to fail fast when the host struct is smaller than the plugin expects.
cucumber_cpp/library/plugin/DynamicLibraryManager.cpp:63LoadFilepushes the library intolibrariesbefore resolvingccr_register/ invoking it. If symbol lookup orccr_registerthrows, the manager keeps a partially-loaded library handle in its list, which can makeGetLoadedLibraries()inaccurate and keep the DLL loaded unexpectedly. Wrap registration in a try/catch andpop_back()on failure to provide a strong exception guarantee.
cucumber_cpp/library/plugin/PLUGIN_ARCHITECTURE.md:370- The
PluginHostContextfield names in this documentation snippet don’t match the actual struct (abiVersion/structSizeinPluginExport.hpp). This can confuse plugin authors and makes the copy/paste example incorrect.
cucumber_cpp/library/Application.cpp:46 - If
manager.Load(paths)throws duringPluginSessionconstruction, the destructor won’t run, leavingDefinitionRegistration/ConverterRegistrysnapshots taken (and possibly partially loaded plugins) without cleanup. Wrappingmanager.Loadin a try/catch that performs the same cleanup as the destructor makes plugin loading failure exception-safe.
support::DefinitionRegistration::Instance().TakeSnapshot();
cucumber_expression::ConverterRegistry::TakeSnapshot();
manager.Load(paths);
}
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.