google / google/googletest-rust
Could we remove the `Copy` requirement of `ActualT` type parameter in `Matcher` trait?
- 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
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