Skip to content
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.

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

Fixes the test gmock_output_test.py with MSVC in a better way #4146

Merged
merged 3 commits into from
May 2, 2023

Conversation

lygstate
Copy link
Contributor

@lygstate lygstate commented Feb 9, 2023

cc @thughes
After this mr:
the std::pair<> can be used directly

And the hacky for

#ifdef _MSC_VER
#define ERROR_DESC "class std::runtime_error"
#else
#define ERROR_DESC "std::runtime_error"
#endif

are no more needed

we can using

std::runtime_error

directly

@asoffer
Copy link
Contributor

asoffer commented Feb 13, 2023

Can you describe what the issue is and why this fix is better (and what it's better than?)

@lygstate
Copy link
Contributor Author

Can you describe what the issue is and why this fix is better (and what it's better than?)

updated the reason

@lygstate
Copy link
Contributor Author

@thughes @asoffer ping

@asoffer
Copy link
Contributor

asoffer commented Feb 21, 2023

Sorry, I'm still not sure I understand what the problem is here. Could you answer:

  • What does gmock_output_test.py do on MSVC?
  • What do you expect it to do?
  • Why is this solution to that problem better than the existing solution?

@lygstate
Copy link
Contributor Author

lygstate commented Feb 22, 2023

Sorry, I'm still not sure I understand what the problem is here. Could you answer:

  • What does gmock_output_test.py do on MSVC?

For MSVC, gmock_output_test.py output struct std::pair<int,bool>, for GCC, it's output
std::pair<int, bool>, it's not the same, my intention is getting these to be same by removing
struct for MSVC's outptu, and strip redundant space for GCC.

  • What do you expect it to do?

ditto.

  • Why is this solution to that problem better than the existing solution?

The new solution can use std::pair<int,bool> directly, the exiting solution is avoid to use it,
this solution would be more robust that output consistence typename.
As a by-product,

#ifdef _MSC_VER
#define ERROR_DESC "class std::runtime_error"
#else
#define ERROR_DESC "std::runtime_error"
#endif

can be simplified to

#define ERROR_DESC "std::runtime_error"

cc @asoffer

Copy link
Contributor

@asoffer asoffer left a comment

Choose a reason for hiding this comment

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

Just a few nits, but otherwise looks great. Thank you!

For MSVC, gmock_output_test.py output struct std::pair<int,bool>, for GCC, it's output
std::pair<int, bool>, it's not the same, my intention is getting these to be same by removing
struct  for MSVC's outptu, and strip redundant space for GCC.

As a by-product,
```
#ifdef _MSC_VER
#define ERROR_DESC "class std::runtime_error"
#else
#define ERROR_DESC "std::runtime_error"
#endif
```
can be simplified to

```
#define ERROR_DESC "std::runtime_error"
```

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
@lygstate
Copy link
Contributor Author

lygstate commented Mar 6, 2023

@asoffer updated, can this be merged now?

@lygstate
Copy link
Contributor Author

@derekmauro @asoffer ping?

@asoffer
Copy link
Contributor

asoffer commented May 1, 2023

Apologies for the delay. Pulling this in.

@copybara-service copybara-service bot merged commit a358018 into google:main May 2, 2023
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