Skip to content

[unittests] Add missing includes #65681

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

Merged
merged 2 commits into from
Sep 8, 2023
Merged

Conversation

zeroomega
Copy link
Contributor

There are missing include and using in TextStubTests and AsmPrinterDwarfTest and they causes build failures when using vanilla GoogleTest v1.14.0. This patch fixes this issue.

There are missing include and using in TextStubTests and
AsmPrinterDwarfTest and they causes build failures when using vanilla
GoogleTest v1.14.0. This patch fixes this issue.
@zeroomega
Copy link
Contributor Author

First time try Github pull request workflow. I hope I done it correctly.

I have a working draft GoogleTest v1.14.0 roll locally when building/testing LLVM under Linux x64. I still need to clean up the code a bit and validate on a few more platforms before I submit it for review.

The "llvm/ADT/SmallString.h" was included in LLVM's patch to GTest header, so it was implicitly included by the test when using LLVM's GTest fork. This causes problem when using a relatively vanilla GTest.

The missing "using testing::DoAll" is a bit puzzling. I am not sure why the build passed without it currently. It causes build failure when using GTest v1.14.0 and I think we should add it.

[unittests] Add missing includes

There are missing include and using in TextStubTests and
AsmPrinterDwarfTest and they causes build failures when using vanilla
GoogleTest v1.14.0. This patch fixes this issue.
Copy link
Collaborator

@pogo59 pogo59 left a comment

Choose a reason for hiding this comment

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

I see only two places that use DoAll in llvm/unittests, and the other one has the using clause, so it makes sense to have it here.

TextStubHelpers.h looks like it doesn't use SmallString, so that include should go in the files where it's actually needed.

@zeroomega
Copy link
Contributor Author

I see only two places that use DoAll in llvm/unittests, and the other one has the using clause, so it makes sense to have it here.

TextStubHelpers.h looks like it doesn't use SmallString, so that include should go in the files where it's actually needed.

The SmallString is used in files from TextStubV1Tests.cpp to TextStubV5Tests.cpp under the same directory of TextStubHelpers.h, for example: https://github.com/llvm/llvm-project/blob/main/llvm/unittests/TextAPI/TextStubV1Tests.cpp . So I think it may be better to add the missing header in the TextStubHelpers.h, which is shared across these unit tests.

Copy link
Collaborator

@pogo59 pogo59 left a comment

Choose a reason for hiding this comment

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

Hmmm normally the project prefers to minimize dependencies, pushing them down into .cpp files rather than up into .h files. But I see that MemoryBuffer.h is included by TextStubHelpers.h even though it's not used there, so in this case I accept that it's reasonable to put SmallString in the .h file.

LGTM

@zeroomega zeroomega merged commit a560d21 into llvm:main Sep 8, 2023
@zeroomega zeroomega deleted the gtest_cleanup branch September 8, 2023 19:11
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