Merge lp://staging/~chasedouglas/xorg-gtest/fixes into lp://staging/~oif-team/xorg-gtest/trunk

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

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

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

I used something called Google and found someone stating that the the pointer to the first element in a vector should:

* Be stable until the array is reallocated
* Should act like an array (i.e. (&vector[0] + 1) == &vector[1])

Though I didn't double-check the C++ standard, these rules pass the sniff test. I rewrote the argument handling to take this into account. I also realized that the strings pointed to by the arguments should still be valid, so there's no real need for the strdup'ing I had in there. The code is much cleaner and compact now.

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

xorg::testing::Environment::Private is not a POD because it contains 3 members (path_to_conf, path_to_server, and Process) that are not POD. The (default) constructor that's being used does not do the wrong thing, but you're almost always better off initializing members with the correct data on construction than to default-construct them and then immediately reset them with different data.

I would still suggest adding an explicit constructor to that class just to initialize it members. It's not a sin to make it explicit.

xorg::testing::Process::Private is a POD and uniform initialization syntax can be used so that it can be initialized in the Process initializer list. Uniform initialization syntax requires C++11 to be enabled in the compiler.

The Process constructor would then become this.

xorg::testing::Process::Process() : d_(new Private{-1}) {
}

I do agree with Stephen's comments.

One minor remark: The file headers still hint to utouch-frame.

> I do agree with Stephen's comments.
>
> One minor remark: The file headers still hint to utouch-frame.

Fixed in https://code.launchpad.net/~thomas-voss/xorg-gtest/Doxygen.

> > 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.
>
> xorg::testing::Environment::Private is not a POD because it contains 3 members
> (path_to_conf, path_to_server, and Process) that are not POD. The (default)
> constructor that's being used does not do the wrong thing, but you're almost
> always better off initializing members with the correct data on construction
> than to default-construct them and then immediately reset them with different
> data.
>
> I would still suggest adding an explicit constructor to that class just to
> initialize it members. It's not a sin to make it explicit.

Fixed.

> xorg::testing::Process::Private is a POD and uniform initialization syntax can
> be used so that it can be initialized in the Process initializer list.
> Uniform initialization syntax requires C++11 to be enabled in the compiler.

I don't want to make this depend on C++11 so that people wanting to test servers on old platforms are excluded. I've left it as is.

I think this is Good Enough.

Looks good to me.