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

Revision history for this message
Chase Douglas (chasedouglas) wrote :

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

« Back to merge proposal