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

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

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

« Back to merge proposal