Merge lp://qastaging/~compiz-team/compiz/compiz.tests_1042537.1 into lp://qastaging/compiz/0.9.8

And I am not sure where all those whitespace changes came from .... WIP.

I fixed a conflict and there are no obvious bugs, but:

1. Copyrights on new files should have your name, not someone else's(?)
2. if (p) / while (p) on pointers is invalid C code. Should use: p != NULL
3. Variables in the middle of a block are not allowed in C, until very recently. But best to be compatible.
4. FIXME needs fixing:
      numTmp = strtol (bit, &end, 10);
   should be something like:
      if ( sscanf(bit, "%ld", &numTmp) == 1 )
5. Still some pointless whitespace changes.

> I fixed a conflict and there are no obvious bugs, but:
>
> 1. Copyrights on new files should have your name, not someone else's(?)

Fixed.

> 2. if (p) / while (p) on pointers is invalid C code. Should use: p != NULL

I've changed this to use != NULL, although this one comes as a surprise to me. Isn't NULL by definition zero and thus if (n < 0 < max) is a good null check?

> 3. Variables in the middle of a block are not allowed in C, until very
> recently. But best to be compatible.

Fixed (though I wonder if we should prefer C99, as we're not using MSVC)

> 4. FIXME needs fixing:
> numTmp = strtol (bit, &end, 10);
> should be something like:
> if ( sscanf(bit, "%ld", &numTmp) == 1 )

Fixed, thanks for the tip.

> 5. Still some pointless whitespace changes.

Fixed.

isUpgrade looks way overcomplicated. There's no reason for it to be 5 lines. It should be more canonical:
     return !strncmp (tokenThree, "upgrade", 7);
It's a minor point, but something to think about every time you write code. Code should always be canonical, in the simplest form possible.

Also, style-wise, upper-case means "this is a macro that is not an addressable function or variable". That's a part of history that I don't think we should try to change. So if it's not a macro, then probably use lower case:
     static const int myconstant = 7;

Tests: Since CCSSettingsUpgradeInternalTest is empty, so is it actually a fixture? If not, then TEST_F should just be TEST.

That all said, I'm tired of all the revisions too. So approved.

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

I think this is pointless:

else if (errno)
    perror ("sscanf");

Still, don't care.

That's fine I've got another branch based on this in progress and will implement your suggestions there.

Generally I prefer keeping constants in uppercase in the tests so that I know that the testing value is the preferred one when coming back to it.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Review: Approve

isUpgrade looks way overcomplicated. There's no reason for it to be 5 lines. It should be more canonical:
     return !strncmp (tokenThree, "upgrade", 7);
It's a minor point, but something to think about every time you write code. Code should always be canonical, in the simplest form possible.

Also, style-wise, upper-case means "this is a macro that is not an addressable function or variable". That's a part of history that I don't think we should try to change. So if it's not a macro, then probably use lower case:
     static const int myconstant = 7;

Tests: Since CCSSettingsUpgradeInternalTest is empty, so is it actually a fixture? If not, then TEST_F should just be TEST.

That all said, I'm tired of all the revisions too. So approved.
--
https://code.launchpad.net/~compiz-team/compiz/compiz.tests_1042537.1/+merge/121641
Your team Compiz Maintainers is subscribed to branch lp:compiz.