[PATCH] D33059: Create an shared include directory for gtest helper functions

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM. Some comments below about the name of the library and getting the linking and other stuff set up correctly, but I'm happy for you to sort that out and land (and fix bots as necessary) w/o going through more rounds of review. =D

One thing I would suggest is mentioning the fact that the particular interface being used for the EXPECT_THAT_ERROR stuff is likely to have further progress to try and improve it, but that this at least consolidates things and provides a clear place for that development to take place. Hopefully that'll help other folks on the commit list understand the context that came up in this review.

Lastly, you should add some warning text somewhere in this code here (maybe a readme near the headers? maybe just in the headers?) that this is intended for use for testing *only*, and shouldn't be depended on by any non-test code. And you should probably add some discussion of this libarry (and the above warning) to the programmer guide for LLVM. But I'm happy for that to be done in subsequent commits where convenient.

Thansk for working on the infrastructure here!



================
Comment at: llvm/lib/Testing/Support/CMakeLists.txt:1
+add_llvm_library(LLVMSupportTesting
+  Error.cpp
----------------
I would call this 'LLVMTestingSupport' to match the directory name I guess?


================
Comment at: llvm/lib/Testing/Support/CMakeLists.txt:8-9
+
+include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
+include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
----------------
Do you need to link gtest and gmock as well? I'm guessing without that a shared build will fail...


https://reviews.llvm.org/D33059