Re: [PATCH xorg-gtest] process: document that va_args for Start() must end with zero-length string

2012-08-16 Thread Chase Douglas 

On 08/14/2012 04:20 PM, Peter Hutterer wrote:

On Tue, Aug 14, 2012 at 10:19:00AM -0700, Chase Douglas wrote:

On 08/13/2012 06:16 PM, Peter Hutterer wrote:

Signed-off-by: Peter Hutterer 
---
  include/xorg/gtest/xorg-gtest-process.h | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/xorg/gtest/xorg-gtest-process.h 
b/include/xorg/gtest/xorg-gtest-process.h
index 69b3b34..8cf60e3 100644
--- a/include/xorg/gtest/xorg-gtest-process.h
+++ b/include/xorg/gtest/xorg-gtest-process.h
@@ -123,7 +123,8 @@ class Process {
 * See 'man execvp' for further information on the variadic argument list.
 *
 * @param program The program to start.
-   * @param args Variadic list of arguments passed to the program.
+   * @param args Variadic list of arguments passed to the program. This list
+   * must end in a zero-length string ("", not NULL).
 *
 * @throws std::runtime_error on failure.
 *
@@ -135,7 +136,8 @@ class Process {
/**
 * Starts a program as a child process.
 *
-   * Takes a variadic list of arguments passed to the program.
+   * Takes a variadic list of arguments passed to the program. This list
+   * must end in a zero-length string ("", not NULL).
 * See 'man execvp' for further information on the variadic argument list.
 *
 * @param program The program to start.



Hmmm... I wish we had though to use a NULL sentinel for the varargs
version. We could have used the gcc sentinel function attribute to
help warn people of bad usage. I suppose it's too late to change now
that we've released it.


I disagree. We're only up to 0.4 and have few users only. Pretending we have
a good and stable API already is optimistic at best, it's better to fix the
things that are obviously (or at least reasonably :) broken. This isn't hard
thing to fix either (I'll send patches out in a bit), so there really isn't
an excuse for having a bad API, it'll just come and haunt us later.


I'll confess that I was attempting a bait a little with my response :). 
I agree that there are few users and the change would be small, so I'm 
fine with fixing this up.


-- Chase
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xorg-gtest] process: document that va_args for Start() must end with zero-length string

2012-08-14 Thread Peter Hutterer 
On Tue, Aug 14, 2012 at 10:19:00AM -0700, Chase Douglas wrote:
> On 08/13/2012 06:16 PM, Peter Hutterer wrote:
> >Signed-off-by: Peter Hutterer 
> >---
> >  include/xorg/gtest/xorg-gtest-process.h | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/include/xorg/gtest/xorg-gtest-process.h 
> >b/include/xorg/gtest/xorg-gtest-process.h
> >index 69b3b34..8cf60e3 100644
> >--- a/include/xorg/gtest/xorg-gtest-process.h
> >+++ b/include/xorg/gtest/xorg-gtest-process.h
> >@@ -123,7 +123,8 @@ class Process {
> > * See 'man execvp' for further information on the variadic argument 
> > list.
> > *
> > * @param program The program to start.
> >-   * @param args Variadic list of arguments passed to the program.
> >+   * @param args Variadic list of arguments passed to the program. This list
> >+   * must end in a zero-length string ("", not NULL).
> > *
> > * @throws std::runtime_error on failure.
> > *
> >@@ -135,7 +136,8 @@ class Process {
> >/**
> > * Starts a program as a child process.
> > *
> >-   * Takes a variadic list of arguments passed to the program.
> >+   * Takes a variadic list of arguments passed to the program. This list
> >+   * must end in a zero-length string ("", not NULL).
> > * See 'man execvp' for further information on the variadic argument 
> > list.
> > *
> > * @param program The program to start.
> >
> 
> Hmmm... I wish we had though to use a NULL sentinel for the varargs
> version. We could have used the gcc sentinel function attribute to
> help warn people of bad usage. I suppose it's too late to change now
> that we've released it.

I disagree. We're only up to 0.4 and have few users only. Pretending we have
a good and stable API already is optimistic at best, it's better to fix the
things that are obviously (or at least reasonably :) broken. This isn't hard
thing to fix either (I'll send patches out in a bit), so there really isn't
an excuse for having a bad API, it'll just come and haunt us later.

Cheers,
   Peter

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xorg-gtest] process: document that va_args for Start() must end with zero-length string

2012-08-14 Thread Chase Douglas 

On 08/13/2012 06:16 PM, Peter Hutterer wrote:

Signed-off-by: Peter Hutterer 
---
  include/xorg/gtest/xorg-gtest-process.h | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/xorg/gtest/xorg-gtest-process.h 
b/include/xorg/gtest/xorg-gtest-process.h
index 69b3b34..8cf60e3 100644
--- a/include/xorg/gtest/xorg-gtest-process.h
+++ b/include/xorg/gtest/xorg-gtest-process.h
@@ -123,7 +123,8 @@ class Process {
 * See 'man execvp' for further information on the variadic argument list.
 *
 * @param program The program to start.
-   * @param args Variadic list of arguments passed to the program.
+   * @param args Variadic list of arguments passed to the program. This list
+   * must end in a zero-length string ("", not NULL).
 *
 * @throws std::runtime_error on failure.
 *
@@ -135,7 +136,8 @@ class Process {
/**
 * Starts a program as a child process.
 *
-   * Takes a variadic list of arguments passed to the program.
+   * Takes a variadic list of arguments passed to the program. This list
+   * must end in a zero-length string ("", not NULL).
 * See 'man execvp' for further information on the variadic argument list.
 *
 * @param program The program to start.



Hmmm... I wish we had though to use a NULL sentinel for the varargs 
version. We could have used the gcc sentinel function attribute to help 
warn people of bad usage. I suppose it's too late to change now that 
we've released it.


Reviewed-by: Chase Douglas 

And pushed as commit 4b8f4dd2b2be2bb036f75403ef4cef31d280f70b.

Thanks!
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xorg-gtest] process: document that va_args for Start() must end with zero-length string

2012-08-13 Thread Peter Hutterer 
Signed-off-by: Peter Hutterer 
---
 include/xorg/gtest/xorg-gtest-process.h | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/xorg/gtest/xorg-gtest-process.h 
b/include/xorg/gtest/xorg-gtest-process.h
index 69b3b34..8cf60e3 100644
--- a/include/xorg/gtest/xorg-gtest-process.h
+++ b/include/xorg/gtest/xorg-gtest-process.h
@@ -123,7 +123,8 @@ class Process {
* See 'man execvp' for further information on the variadic argument list.
*
* @param program The program to start.
-   * @param args Variadic list of arguments passed to the program.
+   * @param args Variadic list of arguments passed to the program. This list
+   * must end in a zero-length string ("", not NULL).
*
* @throws std::runtime_error on failure.
*
@@ -135,7 +136,8 @@ class Process {
   /**
* Starts a program as a child process.
*
-   * Takes a variadic list of arguments passed to the program.
+   * Takes a variadic list of arguments passed to the program. This list
+   * must end in a zero-length string ("", not NULL).
* See 'man execvp' for further information on the variadic argument list.
*
* @param program The program to start.
-- 
1.7.11.2

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel