Re: [PATCH xorg-gtest 09/16] environment: remove default settings

2012-07-05 Thread Chase Douglas 

On 07/04/2012 04:36 AM, Peter Hutterer wrote:

On Tue, Jul 03, 2012 at 10:53:28AM -0700, Chase Douglas wrote:

On 07/02/2012 11:44 PM, Peter Hutterer wrote:

Keep those in the server only, not the environment. And only override the
build-in ones when they've been set by main.

Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
  src/environment.cpp |   22 +-
  1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/environment.cpp b/src/environment.cpp
index b041236..01b2148 100644
--- a/src/environment.cpp
+++ b/src/environment.cpp
@@ -28,7 +28,6 @@
  #include xorg/gtest/xorg-gtest-environment.h
  #include xorg/gtest/xorg-gtest-process.h
  #include xorg/gtest/xorg-gtest-xserver.h
-#include defines.h

  #include sys/types.h
  #include unistd.h
@@ -44,11 +43,8 @@
  #include X11/Xlib.h

  struct xorg::testing::Environment::Private {
-  Private()
-  : path_to_conf(DUMMY_CONF_PATH), path_to_log_file(DEFAULT_XORG_LOGFILE),
-path_to_server(DEFAULT_XORG_SERVER), display(DEFAULT_DISPLAY) {
+  Private() : display(-1) {
}
-
std::string path_to_conf;
std::string path_to_log_file;
std::string path_to_server;
@@ -103,15 +99,23 @@ int xorg::testing::Environment::display() const
  }

  void xorg::testing::Environment::SetUp() {
+  unsigned int display_used;
static char display_string[6];
snprintf(display_string, 6, :%d, d_-display);

-  d_-server.SetDisplayNumber(d_-display);
-  d_-server.SetLogfilePath(d_-path_to_log_file);
-  d_-server.SetConfigPath(d_-path_to_conf);
-  d_-server.Start(d_-path_to_server);
+  if (d_-display = 0)
+d_-server.SetDisplayNumber(d_-display);
+  if (d_-path_to_log_file.length())
+d_-server.SetLogfilePath(d_-path_to_log_file);
+  if (d_-path_to_conf.length())
+d_-server.SetConfigPath(d_-path_to_conf);
+  if (d_-path_to_server.length())
+display_used = d_-server.Start(d_-path_to_server);
+  else
+display_used = d_-server.Start();
d_-server.WaitForConnections();

+  snprintf(display_string, 6, :%d, display_used);
Process::SetEnv(DISPLAY, display_string, true);
  }




Rather than store the values in two places (environment and server
objects), we can simply remove the values from the environment and
manipulate the server directly. Either:

* The environment has a method like:

XServer Environment::XServer();

And then you modify things like:

environment.XServer().SetDisplayNumber(value);

* Provide pass through functions like:

void Environment::SetDisplayNumber(int value) {
   d_-server.SetDisplayNumber(value);
}

And then you modify things like:

environment.SetDisplayNumber(value);

The first approach is a bit easier to code (less boilerplate
pass-through functions), but it violates the Tell, don't ask
policy that many object-oriented APIs follow. I would prefer the
second approach because I think it is easier for the end-user to
follow and understand. They don't have to hunt around in multiple
classes to do what they want to do.


works for me, but what is your need for preserving API atm? Environment and
XServer have two different naming conventions, I decided to stick with the
googletest CamelCase one, environment partially uses the latter.


The standard Google style guide says to use lowercase_underscore for 
variable and member names, and then to use set_variable() and variable() 
for setters and getters. This is one of my least favorite aspects of 
their style guide, but I tend to just try to conform rather than do my 
own thing, so I stuck with it.


Things obviously get murkier when your getter and setter aren't actually 
getting and setting direct members, but maybe are passing the value on 
to another object's getter or setter.



I'm fine with replacing those and align them with XServer, but it may break
your other clients.


If we want to continue using the Google style, then we should follow its 
spirit and use set_display_number(), for example. If we want to break 
with it for getters and setters, then lets do it now and define new 
methods for the old ones, and call the new methods from them. I'm about 
60-40 on the decision to keep with the current format, just because it's 
easier not to have a transition :). But I won't throw a fight if you 
want to change it.


-- 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 09/16] environment: remove default settings

2012-07-05 Thread Peter Hutterer 
On Thu, Jul 05, 2012 at 02:17:30PM -0700, Chase Douglas wrote:
 On 07/04/2012 04:36 AM, Peter Hutterer wrote:
 On Tue, Jul 03, 2012 at 10:53:28AM -0700, Chase Douglas wrote:
 On 07/02/2012 11:44 PM, Peter Hutterer wrote:
 Keep those in the server only, not the environment. And only override the
 build-in ones when they've been set by main.
 
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
   src/environment.cpp |   22 +-
   1 file changed, 13 insertions(+), 9 deletions(-)
 
 diff --git a/src/environment.cpp b/src/environment.cpp
 index b041236..01b2148 100644
 --- a/src/environment.cpp
 +++ b/src/environment.cpp
 @@ -28,7 +28,6 @@
   #include xorg/gtest/xorg-gtest-environment.h
   #include xorg/gtest/xorg-gtest-process.h
   #include xorg/gtest/xorg-gtest-xserver.h
 -#include defines.h
 
   #include sys/types.h
   #include unistd.h
 @@ -44,11 +43,8 @@
   #include X11/Xlib.h
 
   struct xorg::testing::Environment::Private {
 -  Private()
 -  : path_to_conf(DUMMY_CONF_PATH), 
 path_to_log_file(DEFAULT_XORG_LOGFILE),
 -path_to_server(DEFAULT_XORG_SERVER), display(DEFAULT_DISPLAY) {
 +  Private() : display(-1) {
 }
 -
 std::string path_to_conf;
 std::string path_to_log_file;
 std::string path_to_server;
 @@ -103,15 +99,23 @@ int xorg::testing::Environment::display() const
   }
 
   void xorg::testing::Environment::SetUp() {
 +  unsigned int display_used;
 static char display_string[6];
 snprintf(display_string, 6, :%d, d_-display);
 
 -  d_-server.SetDisplayNumber(d_-display);
 -  d_-server.SetLogfilePath(d_-path_to_log_file);
 -  d_-server.SetConfigPath(d_-path_to_conf);
 -  d_-server.Start(d_-path_to_server);
 +  if (d_-display = 0)
 +d_-server.SetDisplayNumber(d_-display);
 +  if (d_-path_to_log_file.length())
 +d_-server.SetLogfilePath(d_-path_to_log_file);
 +  if (d_-path_to_conf.length())
 +d_-server.SetConfigPath(d_-path_to_conf);
 +  if (d_-path_to_server.length())
 +display_used = d_-server.Start(d_-path_to_server);
 +  else
 +display_used = d_-server.Start();
 d_-server.WaitForConnections();
 
 +  snprintf(display_string, 6, :%d, display_used);
 Process::SetEnv(DISPLAY, display_string, true);
   }
 
 
 
 Rather than store the values in two places (environment and server
 objects), we can simply remove the values from the environment and
 manipulate the server directly. Either:
 
 * The environment has a method like:
 
 XServer Environment::XServer();
 
 And then you modify things like:
 
 environment.XServer().SetDisplayNumber(value);
 
 * Provide pass through functions like:
 
 void Environment::SetDisplayNumber(int value) {
d_-server.SetDisplayNumber(value);
 }
 
 And then you modify things like:
 
 environment.SetDisplayNumber(value);
 
 The first approach is a bit easier to code (less boilerplate
 pass-through functions), but it violates the Tell, don't ask
 policy that many object-oriented APIs follow. I would prefer the
 second approach because I think it is easier for the end-user to
 follow and understand. They don't have to hunt around in multiple
 classes to do what they want to do.
 
 works for me, but what is your need for preserving API atm? Environment and
 XServer have two different naming conventions, I decided to stick with the
 googletest CamelCase one, environment partially uses the latter.
 
 The standard Google style guide says to use lowercase_underscore for
 variable and member names, and then to use set_variable() and
 variable() for setters and getters. This is one of my least favorite
 aspects of their style guide, but I tend to just try to conform
 rather than do my own thing, so I stuck with it.
 
 Things obviously get murkier when your getter and setter aren't
 actually getting and setting direct members, but maybe are passing
 the value on to another object's getter or setter.

yes, this is just weird. I just looked at examples and they all had
CamelCase. For reference, the style guide is here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming_Rules

But here's me thinking the whole point of setters and getters is that you
don't have to care what variable is actually set (e.g.  SetDisplayNumber()
vs SetDisplayString(), we may change the backend storage).

 I'm fine with replacing those and align them with XServer, but it may break
 your other clients.
 
 If we want to continue using the Google style, then we should follow
 its spirit and use set_display_number(), for example. If we want to
 break with it for getters and setters, then lets do it now and
 define new methods for the old ones, and call the new methods from
 them. I'm about 60-40 on the decision to keep with the current
 format, just because it's easier not to have a transition :). But I
 won't throw a fight if you want to change it.

I'd rather stick to CamelCaseOnly. Inconsistent function naming is bad
enough already, even worse so when encouraged by the style guide. I'll write

Re: [PATCH xorg-gtest 09/16] environment: remove default settings

2012-07-04 Thread Peter Hutterer 
On Tue, Jul 03, 2012 at 10:53:28AM -0700, Chase Douglas wrote:
 On 07/02/2012 11:44 PM, Peter Hutterer wrote:
 Keep those in the server only, not the environment. And only override the
 build-in ones when they've been set by main.
 
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
   src/environment.cpp |   22 +-
   1 file changed, 13 insertions(+), 9 deletions(-)
 
 diff --git a/src/environment.cpp b/src/environment.cpp
 index b041236..01b2148 100644
 --- a/src/environment.cpp
 +++ b/src/environment.cpp
 @@ -28,7 +28,6 @@
   #include xorg/gtest/xorg-gtest-environment.h
   #include xorg/gtest/xorg-gtest-process.h
   #include xorg/gtest/xorg-gtest-xserver.h
 -#include defines.h
 
   #include sys/types.h
   #include unistd.h
 @@ -44,11 +43,8 @@
   #include X11/Xlib.h
 
   struct xorg::testing::Environment::Private {
 -  Private()
 -  : path_to_conf(DUMMY_CONF_PATH), 
 path_to_log_file(DEFAULT_XORG_LOGFILE),
 -path_to_server(DEFAULT_XORG_SERVER), display(DEFAULT_DISPLAY) {
 +  Private() : display(-1) {
 }
 -
 std::string path_to_conf;
 std::string path_to_log_file;
 std::string path_to_server;
 @@ -103,15 +99,23 @@ int xorg::testing::Environment::display() const
   }
 
   void xorg::testing::Environment::SetUp() {
 +  unsigned int display_used;
 static char display_string[6];
 snprintf(display_string, 6, :%d, d_-display);
 
 -  d_-server.SetDisplayNumber(d_-display);
 -  d_-server.SetLogfilePath(d_-path_to_log_file);
 -  d_-server.SetConfigPath(d_-path_to_conf);
 -  d_-server.Start(d_-path_to_server);
 +  if (d_-display = 0)
 +d_-server.SetDisplayNumber(d_-display);
 +  if (d_-path_to_log_file.length())
 +d_-server.SetLogfilePath(d_-path_to_log_file);
 +  if (d_-path_to_conf.length())
 +d_-server.SetConfigPath(d_-path_to_conf);
 +  if (d_-path_to_server.length())
 +display_used = d_-server.Start(d_-path_to_server);
 +  else
 +display_used = d_-server.Start();
 d_-server.WaitForConnections();
 
 +  snprintf(display_string, 6, :%d, display_used);
 Process::SetEnv(DISPLAY, display_string, true);
   }
 
 
 
 Rather than store the values in two places (environment and server
 objects), we can simply remove the values from the environment and
 manipulate the server directly. Either:
 
 * The environment has a method like:
 
 XServer Environment::XServer();
 
 And then you modify things like:
 
 environment.XServer().SetDisplayNumber(value);
 
 * Provide pass through functions like:
 
 void Environment::SetDisplayNumber(int value) {
   d_-server.SetDisplayNumber(value);
 }
 
 And then you modify things like:
 
 environment.SetDisplayNumber(value);
 
 The first approach is a bit easier to code (less boilerplate
 pass-through functions), but it violates the Tell, don't ask
 policy that many object-oriented APIs follow. I would prefer the
 second approach because I think it is easier for the end-user to
 follow and understand. They don't have to hunt around in multiple
 classes to do what they want to do.

works for me, but what is your need for preserving API atm? Environment and
XServer have two different naming conventions, I decided to stick with the
googletest CamelCase one, environment partially uses the latter.

I'm fine with replacing those and align them with XServer, but it may break
your other clients.

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

[PATCH xorg-gtest 09/16] environment: remove default settings

2012-07-03 Thread Peter Hutterer 
Keep those in the server only, not the environment. And only override the
build-in ones when they've been set by main.

Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
 src/environment.cpp |   22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/environment.cpp b/src/environment.cpp
index b041236..01b2148 100644
--- a/src/environment.cpp
+++ b/src/environment.cpp
@@ -28,7 +28,6 @@
 #include xorg/gtest/xorg-gtest-environment.h
 #include xorg/gtest/xorg-gtest-process.h
 #include xorg/gtest/xorg-gtest-xserver.h
-#include defines.h
 
 #include sys/types.h
 #include unistd.h
@@ -44,11 +43,8 @@
 #include X11/Xlib.h
 
 struct xorg::testing::Environment::Private {
-  Private()
-  : path_to_conf(DUMMY_CONF_PATH), path_to_log_file(DEFAULT_XORG_LOGFILE),
-path_to_server(DEFAULT_XORG_SERVER), display(DEFAULT_DISPLAY) {
+  Private() : display(-1) {
   }
-
   std::string path_to_conf;
   std::string path_to_log_file;
   std::string path_to_server;
@@ -103,15 +99,23 @@ int xorg::testing::Environment::display() const
 }
 
 void xorg::testing::Environment::SetUp() {
+  unsigned int display_used;
   static char display_string[6];
   snprintf(display_string, 6, :%d, d_-display);
 
-  d_-server.SetDisplayNumber(d_-display);
-  d_-server.SetLogfilePath(d_-path_to_log_file);
-  d_-server.SetConfigPath(d_-path_to_conf);
-  d_-server.Start(d_-path_to_server);
+  if (d_-display = 0)
+d_-server.SetDisplayNumber(d_-display);
+  if (d_-path_to_log_file.length())
+d_-server.SetLogfilePath(d_-path_to_log_file);
+  if (d_-path_to_conf.length())
+d_-server.SetConfigPath(d_-path_to_conf);
+  if (d_-path_to_server.length())
+display_used = d_-server.Start(d_-path_to_server);
+  else
+display_used = d_-server.Start();
   d_-server.WaitForConnections();
 
+  snprintf(display_string, 6, :%d, display_used);
   Process::SetEnv(DISPLAY, display_string, true);
 }
 
-- 
1.7.10.4

___
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 09/16] environment: remove default settings

2012-07-03 Thread Chase Douglas 

On 07/02/2012 11:44 PM, Peter Hutterer wrote:

Keep those in the server only, not the environment. And only override the
build-in ones when they've been set by main.

Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
  src/environment.cpp |   22 +-
  1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/environment.cpp b/src/environment.cpp
index b041236..01b2148 100644
--- a/src/environment.cpp
+++ b/src/environment.cpp
@@ -28,7 +28,6 @@
  #include xorg/gtest/xorg-gtest-environment.h
  #include xorg/gtest/xorg-gtest-process.h
  #include xorg/gtest/xorg-gtest-xserver.h
-#include defines.h

  #include sys/types.h
  #include unistd.h
@@ -44,11 +43,8 @@
  #include X11/Xlib.h

  struct xorg::testing::Environment::Private {
-  Private()
-  : path_to_conf(DUMMY_CONF_PATH), path_to_log_file(DEFAULT_XORG_LOGFILE),
-path_to_server(DEFAULT_XORG_SERVER), display(DEFAULT_DISPLAY) {
+  Private() : display(-1) {
}
-
std::string path_to_conf;
std::string path_to_log_file;
std::string path_to_server;
@@ -103,15 +99,23 @@ int xorg::testing::Environment::display() const
  }

  void xorg::testing::Environment::SetUp() {
+  unsigned int display_used;
static char display_string[6];
snprintf(display_string, 6, :%d, d_-display);

-  d_-server.SetDisplayNumber(d_-display);
-  d_-server.SetLogfilePath(d_-path_to_log_file);
-  d_-server.SetConfigPath(d_-path_to_conf);
-  d_-server.Start(d_-path_to_server);
+  if (d_-display = 0)
+d_-server.SetDisplayNumber(d_-display);
+  if (d_-path_to_log_file.length())
+d_-server.SetLogfilePath(d_-path_to_log_file);
+  if (d_-path_to_conf.length())
+d_-server.SetConfigPath(d_-path_to_conf);
+  if (d_-path_to_server.length())
+display_used = d_-server.Start(d_-path_to_server);
+  else
+display_used = d_-server.Start();
d_-server.WaitForConnections();

+  snprintf(display_string, 6, :%d, display_used);
Process::SetEnv(DISPLAY, display_string, true);
  }




Rather than store the values in two places (environment and server 
objects), we can simply remove the values from the environment and 
manipulate the server directly. Either:


* The environment has a method like:

XServer Environment::XServer();

And then you modify things like:

environment.XServer().SetDisplayNumber(value);

* Provide pass through functions like:

void Environment::SetDisplayNumber(int value) {
  d_-server.SetDisplayNumber(value);
}

And then you modify things like:

environment.SetDisplayNumber(value);

The first approach is a bit easier to code (less boilerplate 
pass-through functions), but it violates the Tell, don't ask policy 
that many object-oriented APIs follow. I would prefer the second 
approach because I think it is easier for the end-user to follow and 
understand. They don't have to hunt around in multiple classes to do 
what they want to do.


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