Code review comment for lp://staging/~chasedouglas/xorg-gtest/fixes

Revision history for this message
Stephen M. Webb (bregma) wrote :

(1) "make install" doesn't install the headers.

(2) in xorg::testing::Environment::SetUp(), the first parameter is a std::string, but std::string::c_str() is passed.

(3) the error messages in xorg::testing::Environment::TearDown() are missing newlines.

(4) xorg::testing::Process::Start() does a lot of simulating the new operator by using malloc() followed by a 'throw bad_alloc' on failure: argv at least should be a std::vector<char*>, eliminating all mallocs and reallocs.

(5) can the pimpls be made std::unique_ptrs instead of using explicit deletes (this is not a big thing).

(6) either a constructor should be added to xorg::testing::Environment::Private or a uniform initializer should be used in the initialization list in the xorg::testing::Environment constructor when initializing Private members (it's bad form to initialize someone else's members). Same goes for xorg::testing::Process::Private.

(7) Why use the C-compatibility headers (eg. string.h, stdlib.h, errno.h) instead of the C++ headers (<cstring>, <cstdlib>, <cerrno>) if this is C++ code? It's not broken, it's just a little inconsistent.

But when all is said and done, this works well.

review: Needs Fixing

« Back to merge proposal