Introduce a new check to upgrade user code based on API changes in Googletest.
The check finds uses of old Googletest APIs with "case" in their name and replaces them with the new APIs named with "suite".
I think will be good idea to replace upgrade with modernize to be consistent with similar checks in other module.
Function should be static instead on placed in anonymous namespace. See LLVM Coding Guidelines. Same for other places.
Please remove unnecessary empty line.
Please remove space before colon.
You need to upload the entire patch, not as compared to the previous patch.
Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.
I will work on changing this in the next diff update, if your opinion holds. The context for currently using "upgrade" is:
a) The modernize module seems to contain checks that are geared at using more modern language features as opposed to API breaking changes. With some exceptions, they seem to be about changing from old to new practice while both ways remain valid code. The goal of this check is to take code that will break with future googletest API changes and turn it into working code, allowing users to upgrade the version of googletest they use. Maybe upgrade is not the best term, but I don't think modernize is a better fit.
b) The check is meant to be consistent with abseil-upgrade-duration-conversions, which is aimed to upgrade code before API breaking changes. So if we change this check, I would like to change the Abseil one as well. Maybe this would be better done together in a follow-up patch?
I don't feel very strongly, just thought it would be good to provide the context. Please let me know how you'd like me to proceed.
Generally this LGTM.
I'll take another pass after the comments are addressed.
Could you rename this to better represent what the diagnostic text is saying, rather than just being diagnostic text?
Why not put this inside the unnamed namespace instead?
Same with the function below.
Instead of walking the tree, can't you ask if the Node is dependent in some way?
Maybe add an assertion for this comment?
https all the things!
You probably don't have to test for the full message every time. but I don't know it matters either way.
Sorry, the original comment got lost between me updating two patches as I noticed I did it wrong without seeing your comment.
Unfortunately there are no versioning macros in googletest, so I'm not sure how to keep this check from providing warnings if it is run while depending on a version that is too early. The new names are in master and will be part of an upcoming version 1.9 release. I have tried to update the documentation to clarify which version of googletest this intended for. Please let me know how you think we should proceed.
Thanks for catching this, was having issues in the diff.
I had a previous comment that llvm coding guidelines prefer static and like to keep anonymous namespaces as small as possible and only around type declarations.
Thanks for spotting this. There is some inconsistency within the project. The top of the github repository is named "Googletest" with directories being "googletest" instead of "google_test", but documentation does use "Google Test".
For now I have changed the message to use "Google Test", but have kept "googletest" in the check name. Let me know if you would prefer "GoogleTest" and "google-test" elsewhere as well.
I think it is a trade-off. Given where this gets called, what we are really looking to gather are all places that are inside a template that are not instantiation dependent. If we only use a check against instantiation dependent, then the hash set gets a lot bigger because it holds every matched location and is not filtered down to the ones inside templates. Let me know if you think I should explore that route instead.
I'm not fully current on what can and can't be done in clang-tidy, but i suppose if the
check has matched something, then it must mean that the googletest headers were parsed.
Can you perhaps look in the AST that the new names are present?
E.g. the new macros, specified in getNewMacroName().
Yes, makes sense. Let me know what you think of the approach I've added in the latest diff. I think it is sufficient to check for the existence of one of the new macros in the right file and one new method in each matched class.
The tests as they are cover the positive case in that they would not show warning or fixes if we didn't find the replacements. The only way to test the negative is to make a second test with a second set of mock googletest headers that declare things in the v1.8 way. Is this what you would prefer?
Looks reasonable. I did not review the check itself though.
Are test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp and test/clang-tidy/google-upgrade-googletest-case.cpp identical other than the included header and expected output?
I'd recommend to condense it into a single file, and just have two RUN lines each one checking different message prefixes
Have you tried llvm::DenseSet instead?
|9 ↗||(On Diff #206855)|
The nosuite test was a strict subset. I've combined them with a few lines that needed to be turned via preprocessor branches. Thank you, I actually wasn't aware that these tests can have multiple RUN lines.
Thanks, I wasn't aware of what is available.
|9 ↗||(On Diff #206855)|
I added a comment to better explain this in the combined test. check_clang_tidy.py asserts that there is at least one message, fix or note comment present in the file. If there isn't one, the script returns without even running the test. I couldn't see an option to pass that would turn of this check, please let me know if you are aware of one.