Avoid Mimick argument matching for unconditional patches - #3221
Conversation
Avoid instantiating Mimick's generic argument matcher when a test patch applies to every invocation. This prevents Clang from compiling ordered comparisons for function pointer arguments and allows the temporary warning suppression to be removed. Signed-off-by: Ulaş <36420486+ukis666@users.noreply.github.com>
|
Tick the box to add this pull request to the merge queue (same as
|
| 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); |
There was a problem hiding this comment.
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...
| 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); |
There was a problem hiding this comment.
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_?
can you disclose which AI tools and models are used to create this PR? |
|
according to the test, i think we need to do the test with source build instead of released pacakges. |
2 participants
Description
mocking_utils::Patchcurrently configures replacements throughmmk_when(..., mmk_any(...)). Although each argument is matched asANY,Mimick still instantiates its generic comparison code for the argument types.
For
rcl_logging_configure_with_output_handler, one of those arguments is afunction pointer. Clang therefore diagnoses the generated ordered comparisons
with
-Wordered-compare-function-pointers.This change avoids instantiating Mimick's generic argument-matcher path when
mocking_utils::Patchinstalls an unconditional replacement. Instead, thereplacement trampoline is installed directly through Mimick's mock creation
API.
Patchdoes not expose selective argument matching; its current use configuresa replacement for every invocation. This preserves the intended unconditional
replacement behavior while avoiding Mimick's generic argument-matcher path.
With that matcher path no longer instantiated, function-pointer arguments do
not generate ordered comparisons and the Clang-specific diagnostic suppression
in
test_utilities.cppcan be removed.Fixes #2488
Is this user-facing behavior change?
No. This only changes the internal test mocking utility.
Did you use Generative AI?
Yes. Generative AI was used to assist with analysis and validation of the change.
Additional Information
The change was validated against ROS 2 Rolling using
rostooling/setup-ros-docker:ubuntu-resolute-ros-rolling-ros-base-latest.test_utilitiesbuilds and passes with Clang 21.1.8 and-Werror=ordered-compare-function-pointers.rclcpptest build completes successfully with the same warningpromoted to an error.
control run in the same environment reports the same 3 failures:
test_client,test_subscription, andtest_subscription_options.No additional failures were introduced by this change in the validation
environment.
test_utilitiesalso builds and passes with GCC.Upstream CI can provide additional validation across the supported platform
matrix.