[PATCH xorg-gtest 06/11] process: require NULL as last argument to Start()

[Chase Douglas]

On 08/15/2012 11:36 PM, Peter Hutterer wrote:
> And handle empty arguments and NULL_terminated arguments. This is a better
> API than require the user to pass empty strings, especially since in some
> cases an empty string may be a valid argument.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>   include/xorg/gtest/xorg-gtest-process.h |  7 ++++---
>   src/process.cpp                         | 13 ++++++++-----
>   2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/xorg/gtest/xorg-gtest-process.h b/include/xorg/gtest/xorg-gtest-process.h
> index 48f387b..b4ac0d9 100644
> --- a/include/xorg/gtest/xorg-gtest-process.h
> +++ b/include/xorg/gtest/xorg-gtest-process.h
> @@ -30,6 +30,7 @@
>   #define XORG_GTEST_PROCESS_H
>
>   #include <stdarg.h>
> +#include <X11/Xfuncproto.h> /* for _X_SENTINEL */
>
>   #include <memory>
>   #include <string>
> @@ -114,7 +115,7 @@ class Process {
>      *
>      * @param program The program to start.
>      * @param args Variadic list of arguments passed to the program. This list
> -   * must end in a zero-length string ("", not NULL).
> +   * must end with NULL.
>      *
>      * @throws std::runtime_error on failure.
>      *
> @@ -127,7 +128,7 @@ class Process {
>      * Starts a program as a child process.
>      *
>      * Takes a variadic list of arguments passed to the program. This list
> -   * must end in a zero-length string ("", not NULL).
> +   * must end with NULL.
>      * See 'man execvp' for further information on the variadic argument list.
>      *
>      * @param program The program to start.
> @@ -137,7 +138,7 @@ class Process {
>      * @post If successful: Child process forked and program started.
>      * @post If successful: Subsequent calls to Pid() return child process pid.
>      */
> -  void Start(const std::string& program, ...);
> +  void Start(const std::string& program, ...) _X_SENTINEL(0);
>
>     /**
>      * Terminates (SIGTERM) this child process and waits a given timeout for
> diff --git a/src/process.cpp b/src/process.cpp
> index 03dbc42..b85170b 100644
> --- a/src/process.cpp
> +++ b/src/process.cpp
> @@ -69,7 +69,6 @@ void xorg::testing::Process::Start(const std::string &program, const std::vector
>       args.push_back(strdup(program.c_str()));
>
>       for (it = argv.begin(); it != argv.end(); it++)
> -      if (!it->empty())
>           args.push_back(strdup(it->c_str()));

This should be de-indented now ^^

>       args.push_back(NULL);
>
> @@ -82,10 +81,14 @@ void xorg::testing::Process::Start(const std::string &program, const std::vector
>   void xorg::testing::Process::Start(const std::string& program, va_list args) {
>     std::vector<std::string> argv;
>
> -  do {
> -    std::string arg(va_arg(args, char*));
> -    argv.push_back(arg);
> -  } while (!argv.back().empty());
> +  if (args) {
> +    char *arg;
> +    do {
> +      arg = va_arg(args, char*);
> +      if (arg)
> +        argv.push_back(std::string(arg));
> +    } while (arg);
> +  }
>
>     Start(program, argv);
>   }
>

Thanks for fixing this up :).

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>