google / google/googletest-rust

Could we remove the `Copy` requirement of `ActualT` type parameter in `Matcher` trait?

Open
#542 4 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
432
Forks
36
Avg merge
7d 17h
Merged PRs (30d)
2

Description

In 471d4a2, the `ActualT` changed from an associated type to a type parameter. I think it is a good change, but is `ActualT: Copy` still required? In #323, there was some discussions about lifetime problems, but I don't think that is still valid now because the lifetime of the object being matched is not tied to the matcher anymore.

The comment in the code mentioned:

```rust
// `ActualT` requires `Copy` so that `actual` could be passed to `matches` and
// if it fails passed to `explain_match`. We can relax this constraint later by
// requiring only `Clone`.
```

But `Copy` in rust is generally considered cheap (if object is so large that memcpy is expensive, then the type probably shouldn't implement Copy in the first place), so there is no harm to take `actual: &ActualT` instead. If `ActualT` is `Copy`, the implementer can dereference it by themselves. This simplify a lot of lifetime problems when implementing custom matchers.

Contributor guide

Open the contributing guide

Research direction

Locate the Matcher trait and the comment describing the ActualT: Copy requirement. Trace how actual is passed to matches and explain_match, then review the lifetime concerns discussed in #323. Done means the constraint can be removed or relaxed without reintroducing those lifetime problems, with the relevant matcher tests still passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.