Skip to content

Avoid Mimick argument matching for unconditional patches #3221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ukis666 wants to merge 1 commit into
base: rolling
Choose a base branch
from
+13 −35
Open

Avoid Mimick argument matching for unconditional patches #3221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 13 additions & 22 deletions rclcpp/test/mocking_utils/patch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,23 +386,21 @@ template<size_t ID, typename ReturnT, typename ... ArgTs>
class Patch<ID, ReturnT(ArgTs...)>
{
public:
using mock_type = typename PatchTraits<ID, ReturnT(ArgTs...)>::mock_type;

/// Construct a patch.
/**
* \param[in] target Symbol target string, using Mimick syntax
* i.e. "symbol(@scope)?", where scope may be "self" to target the current
* binary, "lib:library_name" to target a given library, "file:path/to/library"
* to target a given file, or "sym:other_symbol" to target the first library
* that defines said symbol.
* \param[in] proxy An indirection to call the target function.
* This indirection must ensure this call goes through the function's
* trampoline, as setup by the dynamic linker.
* \param[in] proxy An indirection retained for function signature deduction.
* \return a mocking_utils::Patch instance.
*/
explicit Patch(const std::string & target, std::function<ReturnT(ArgTs...)> proxy)
: target_(target), proxy_(proxy)
explicit Patch(
const std::string & target, std::function<ReturnT(ArgTs...)> proxy)
: target_(target)
{
(void)proxy;
}

// Copy construction and assignment are disabled.
Expand Down Expand Up @@ -446,35 +444,28 @@ class Patch<ID, ReturnT(ArgTs...)>
}

private:
// Helper for template parameter pack expansion using `mmk_any`
// macro as pattern.
template<typename T>
T any() {return mmk_any(T);}

void replace_with(std::function<ReturnT(ArgTs...)> replacement)
{
if (mock_) {
throw std::logic_error("Cannot configure patch more than once");
}
auto type_erased_trampoline =
reinterpret_cast<mmk_fn>(prepare_trampoline<ID>(replacement));
auto MMK_MANGLE(mock_type, create) =
PatchTraits<ID, ReturnT(ArgTs...)>::MMK_MANGLE(mock_type, create);
mock_ = mmk_mock(target_.c_str(), mock_type);
mmk_when(proxy_(any<ArgTs>()...), .then_call = type_erased_trampoline);
struct mmk_mock_options options {};
options.sentinel_ = 1;
mock_ = mmk_mock_create_internal(target_.c_str(), type_erased_trampoline, options);

@fujitatomoya fujitatomoya Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a broken patch could never go unnoticed before.
the raw internal call sidesteps that safety net, i believe.
it can return an invalid handle, mock_ stays useless, and replace_with returns as if everything worked.
the consequence is nasty specifically because this is a test utility...

@fujitatomoya fujitatomoya Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i do not think using internal methods and members are correct thing to do here. could it use public stub APIs instead of mmk_mock_create_internal especially with options.sentinel_?

}

mock_type mock_{nullptr};
mmk_fn mock_{nullptr};
std::string target_;
std::function<ReturnT(ArgTs...)> proxy_;
};

/// Make a patch for a `target` function.
/**
* Useful for type deduction during \ref mocking_utils::Patch construction.
*
* \param[in] target Symbol target string, using Mimick syntax.
* \param[in] proxy An indirection to call the target function.
* \param[in] proxy An indirection used for function signature deduction.
* \return a mocking_utils::Patch instance.
*
* \tparam ID Numerical identifier for this patch. Ought to be unique.
Expand All @@ -490,8 +481,8 @@ auto make_patch(const std::string & target, std::function<SignatureT> proxy)

/// Define a dummy operator `op` for a given `type`.
/**
* Useful to enable patching functions that take arguments whose types
* do not define basic comparison operators, as required by Mimick.
* Retained for compatibility with tests that define comparison operators
* for types passed through the mocking utility.
*/
#define MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(type_, op) \
template<typename T> \
Expand All @@ -509,7 +500,7 @@ auto make_patch(const std::string & target, std::function<SignatureT> proxy)

/// A transparent forwarding proxy to a given `function`.
/**
* Useful to ensure a call to `function` goes through its trampoline.
* Useful for function signature deduction while preparing a patch.
*/
#define MOCKING_UTILS_PATCH_PROXY(function) \
[] (auto && ... args)->decltype(auto) { \
Expand Down
13 changes: 0 additions & 13 deletions rclcpp/test/rclcpp/test_utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,7 @@
#include "rclcpp/exceptions.hpp"
#include "rclcpp/utilities.hpp"

#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wordered-compare-function-pointers"
#endif
// TODO(ahcorde): the function mocking_utils::patch_and_return called with
// rcl_logging_configure_with_output_handler is returning: "Comparison between pointer and integer"
// Disabling this warning is fine for now.
// Related issue https://github.com/ros2/rclcpp/issues/2488
#include "../mocking_utils/patch.hpp"

#ifdef __clang__
#pragma clang diagnostic pop
#endif

#include "../utils/rclcpp_gtest_macros.hpp"

TEST(TestUtilities, remove_ros_arguments) {
Expand Down