Skip to content

Added StrEq matcher.#256

Merged
FranckRJ merged 1 commit intodev-2.1.0from
strcmp-matchers
Feb 27, 2022
Merged

Added StrEq matcher.#256
FranckRJ merged 1 commit intodev-2.1.0from
strcmp-matchers

Conversation

@FranckRJ
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Aug 22, 2021

Coverage Status

Coverage increased (+0.002%) to 99.924% when pulling 5529f49 on strcmp-matchers into f7fe56f on dev-2.1.0.

@FranckRJ
Copy link
Collaborator Author

Oh i didn't tested the formatting of the error of the matcher.

When(Method(mock, strfunc).Using(StrEq("second"))).Return(2);

SomeInterface &i = mock.get();
ASSERT_EQUAL(1, i.strfunc("first"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work if the const char* is pointing to a temporary, instead of a literal?

Copy link
Collaborator Author

@FranckRJ FranckRJ Dec 14, 2021

Choose a reason for hiding this comment

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

I looked a bit more, matchers aren't really design to hold copy of stuff, they just hold references. The issue is the same for all matchers, so I won't change it only for this one.

Like others matchers, it's up to the user to ensure that the references that are passed stay valid for the lifetime of the matcher.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like you said in another message maybe it could be improved depending of the value category of the variable but it's outside of the scope of this PR i think.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah - I see no problem. Could be a future improvement. (I'm just seeing lots of scope for small improvements in this library in general)

Copy link
Contributor

Choose a reason for hiding this comment

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

Re "references" in general. There are a number of cases where Fakeit stops temporaries and/or rvalue refs from being used (this one, const char*, is less likely to have issues with temporaries though).
And so sometimes makes the tests a little more complicated than necessarily, and adds more gotchas, where it might be possible to avoid.

@FranckRJ
Copy link
Collaborator Author

FranckRJ commented Nov 22, 2021 via email

@malcolmdavey
Copy link
Contributor

If the temporary is not valid anymore, then no. But I guess it's a reasonable requirement for this kind of stuff, no ? Or do you think I should strdup the string to store it somewhere ?

Short answer - yes, ideally.

Just for clarity, as I was not sure if I was clear. Was referring to the Using lines, and other if it made sense to use a copy.

When(Method(mock, strfunc).Using(StrEq("first"))).Return(1);

I think most of the time you can get away without the copy, but also thinking of how to make the library more complete/forgiving, and not always needing to worry about cases of potentially dangling references.

Had been looking at the other main case where we currently only story references, and was thinking if and how we could find ways to store a copy (either in some cases implicitly where we can detect it makes sense - e.g. revalue ref, or explicitly - will working on some ideas for it).

@FranckRJ
Copy link
Collaborator Author

FranckRJ commented Nov 22, 2021 via email

@malcolmdavey
Copy link
Contributor

malcolmdavey commented Nov 22, 2021

Also I haven't really tested out the Using yet, so don't know when we might be able to get some automatic handling/conversion happening.
Currently testing out just the Return and already found a few things there that can be simply improved.

@FranckRJ FranckRJ marked this pull request as ready for review February 27, 2022 15:33
@FranckRJ FranckRJ merged commit f03df70 into dev-2.1.0 Feb 27, 2022
@FranckRJ FranckRJ deleted the strcmp-matchers branch February 27, 2022 16:34
@FranckRJ FranckRJ mentioned this pull request Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants