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

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

It does here... Any ideas why it wasn't working on your system?

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

Fixed.

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

Fixed.

> (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.

Unfortunately, I can't find a version of exec* that takes an va_list. The two main versions are execl, which is variadic, and execv, which takes a typical argv list. There isn't a way to programatically call a variadic function, so that leaves us having to create an array.

While it would be nice if we could use std::vector, there's no method that provides a c-style array like std::string::data(). My first attempt was to copy all the arrays into a std::vector, and then use the size of the std::vector to allocate the argv array. This seemed like a bunch of duplicitous effort, so I wrote a pure C version. I'm not sure which is better, but I don't think it makes sense to switch it from one annoying implementation to another.

Any ideas on how to make this better?

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

Fixed.

> (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.

The private members are POD structs. They don't have privacy, and they don't have methods. It seems overkill to make them full classes. And because these are just pimpls, it's not really the same as one normal class initializing another normal class' members.

Do you think we should make them full classes?

> (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.

Fixed.

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

Great!