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

Remove gtest-type-util.h.pump and move pump to googlemock #2388

Merged
merged 5 commits into from
Oct 29, 2019

Conversation

kuzkry
Copy link
Contributor

@kuzkry kuzkry commented Aug 12, 2019

Yes, I'm sane XD

I think there's no need to generate (using a pump) most of the code in googletest/include/gtest/internal/gtest-internal.h. Actually, this is the only thing generated in the googletest module, so I've additionally moved the pumping script to the googlemock directory.

What can be said is:

  1. I consider this pull request as a complete feature so I wouldn't like to split it into multiple pull requests
  2. I think my version is more readable (it's short and fits the screen), end of story, full stop :)
  3. Leaves us with not having to maintain so much both generated and pump code
  4. Does not break any user code. Changes are mostly done within the internal namespace and the ones that aren't internal (e.g. macros) were modified, so that it's not possible to break it
  5. Does not have workarounds for generating nicer compiler errors. By the way, e.g. redefined type list is designed to never fail, if a user provides at least one type
  6. Unfortunately "Templates" family of classes may still fail with ugly compiler errors
  7. Uses C++11 variadic macros and no template magic tricks(!)
  8. On my machine average compilation times for a single thread using all gtest & gmock unit tests enabled dropped down (roughly by 2%):
  • base (master) -> 4m 41s152
  • 1st commit -> 4m 38s228
  • 2nd commit -> 4m 35s697
  1. Uses a common logic, so now passing a single type, e.g. TYPED_TEST_SUITE(FooTest, int); will internally generate TypeList<int>. This is why a "plumbing" unit test had to be adjusted. It think it makes it easier to handle, though it's a small trade-off.
  2. Both Types and Templates reuse the same None struct (this changed meaning; now it's a marker that nothing is there, previously it was Types0 and Templates0).
  3. Since variadic templates are used, there is no limit in the number of input types.

If something is seriously broken or you didn't understand, please make a comment.

Thanks for reading it and happy reviewing :)

@kuzkry kuzkry changed the title Remove gtest type util.pump and move pump to googlemock Remove gtest-type-util.h.pump and move pump to googlemock Aug 12, 2019
@kuzkry kuzkry force-pushed the remove-gtest-type-util.pump branch from 6c21fb6 to 8ad1cb3 Compare August 12, 2019 15:24
@EricWF
Copy link

EricWF commented Aug 13, 2019

Thank you for your work improving Google test.

I appreciate the through commit message describing each change. Could you please divide this into multiple requests so it's easier to review.

How'd thoroughly has this been tested? many more millions of tests are written with Google tests that are written against it. do you think this will break people? if so what is the scale of the breakage?

@kuzkry
Copy link
Contributor Author

kuzkry commented Aug 13, 2019

Thank you for your work improving Google test.

I appreciate the through commit message describing each change. Could you please divide this into multiple requests so it's easier to review.

How'd thoroughly has this been tested? many more millions of tests are written with Google tests that are written against it. do you think this will break people? if so what is the scale of the breakage?

About splitting: I wouldn't like to do that and it's for your own good. GitHub doesn't detect related changes even if you place a single commit on top of another from another pull request. Example:
#2323 (there were 6 commits there)
#2326 (there was only one commit on top of these 6, but GitHub shows 7 commits)
I wish it worked so flawlessly as e.g. Gerrit does. With this pull request, I don't have any other idea than recommending you to check it commit-wise. Or maybe you can call me on Hangouts / Skype or something else, how about that? With +68 lines, it wouldn't take much time :)

About tests and compatibility: I didn't write it over night :) It took me several days of planning, coding and double checking. This code is really well tested with current test suite. Also name generators work like they used to, thus the output looks exactly the same.
This will break code of those who depend on the internal implementation marked not to use anyway. Others won't notice anything.

@kuzkry kuzkry force-pushed the remove-gtest-type-util.pump branch from 8ad1cb3 to 8068979 Compare August 16, 2019 06:46
@kuzkry kuzkry force-pushed the remove-gtest-type-util.pump branch 2 times, most recently from 88a86a3 to f9df3e4 Compare September 3, 2019 16:17
@gennadiycivil
Copy link
Contributor

273990769
Thank you, we have started internal review. Please don't push any more changes into this PR as they might be overwritten.
This one will probably take a while

@gennadiycivil gennadiycivil self-assigned this Oct 10, 2019
@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 11, 2019

Cool @gennadiycivil. Numbers are big, but the idea is similar (you will spot the pattern). I suggest going commit-wise (each is compilable, self-contained) or / and encourage you to ask me questions.

@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 25, 2019

@gennadiycivil, I've got a conflict in googletest/scripts/pump.py after the latest commit 37f3227. Would you like me to resolve it? Because you previously wrote:

Please don't push any more changes into this PR as they might be overwritten.

@gennadiycivil
Copy link
Contributor

@kuzkry It is better if you re-base and run against your tests. Could you please accept this version of pump into your branch and run your tests.
It would be good to have the confirmation that this version of pump still works with your PR as per your testing.

Thank you!

googletest/scripts/ and googlemock/scripts/ were the remnants of the past - these were not synched to the internal systems. I had to ensure that these directories are synched in preparation to move the pump.py. I had to assume internal version of pump to be the source of truth.

I have found some internal usages of pump accumulated over the years that I am cleaning up before we can accept this PR and move the pump into googlemock.

Longer term pump needs to disappear, there is really no need for it now.

@kuzkry kuzkry force-pushed the remove-gtest-type-util.pump branch from f9df3e4 to 1a49b67 Compare October 25, 2019 15:04
@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 25, 2019

@gennadiycivil, thanks :) As for the script - again I'm trying to move it only, not to modify it so I fully accepted the one from master.

vslashg added a commit that referenced this pull request Oct 28, 2019
vslashg added a commit that referenced this pull request Oct 29, 2019
vslashg added a commit that referenced this pull request Oct 29, 2019
@vslashg vslashg merged commit 1a49b67 into google:master Oct 29, 2019
@kuzkry kuzkry deleted the remove-gtest-type-util.pump branch October 30, 2019 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants