Code review comment for lp:~dandrader/geis/lp984069

* xorg-gtest.m4 shouldn't be modified. It comes from upstream xorg-gtest and may be replaced by a newer version from an updated xorg-gtest automatically by aclocal --install. IIRC, I think it may also be always replaced by our packaging since it uses aclocal --install --force.

Since _CHECK_GTEST is meant to be an internal implementation detail of xorg-gtest.m4, I suggest copying it to a new file m4/gtest.m4 and removing the leading underscore.

* libgtest_geis_a_CPPFLAGS still needs $(GTEST_CPPFLAGS). If the gtest headers are installed somewhere other than a standard location, GTEST_CPPFLAGS will have the -I<header path> option. This will be needed for building xorg-gtest too. Likewise for gtest_geis2_grail_backend_CPPFLAGS.

* libgtest_geis_a_LIBADD should add the libgtest.a file instead of an intermediate object file. I don't know if that will work, but the .o file is sort of an autotools implementation detail. This may also remove the need for libgtest_geis_a_DEPENDENCIES.

* libgtest doesn't need to be built with --std=c++0x. xorg-gtest doesn't need to built with it either. I also don't know why TEST_ROOT_DIR is defined for both gtest libs. I think it was a copy/paste error.

* PRINT_FAILURE_AND_RETURN isn't necessary. All you need is ADD_FAILURE() << [message]; The file and line will be printed automatically, and the function return automatically.

* I like the idea of MOCK_PRINT. I think it duplicates the geis logging, though. Why not use geis_debug()? I know this means you can't enable mock printouts without geis debug printouts, but I think that's ok. If you're debugging at this level you might be setting GEIS_DEBUG=3 anyway. If you have a standard format where something like "XMOCK: " is prepended to the message, it will be easy to grep or grep -v on it.