skdiff: replace --failonmismatches with repeatable --failonresult option

skdiff: replace --failonmismatches with repeatable --failonresult option
Committed: https://code.google.com/p/skia/source/detail?r=4611

[epoger, 2012-07-12 22:19:18]

I would like to get LGs from *both* of you before committing this... have I
finally pushed skdiff_main.cpp over the edge?

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp
File tools/skdiff_main.cpp (right):

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode60
tools/skdiff_main.cpp:60: Result getResultByName(const char *name) {
I should probably pull Result into its own class file, but I'd like to do that
in a later CL if that's OK.

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode1172
tools/skdiff_main.cpp:1172: if (!failOnTheseResultTypes.contains(type)) {
This depends on my pending https://codereview.appspot.com/6349103/ ('Add
SkTDArray.contains(), which is just a wrapper around find()')

[TomH, 2012-07-13 13:02:12]

LGTM.

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp
File tools/skdiff_main.cpp (right):

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode60
tools/skdiff_main.cpp:60: Result getResultByName(const char *name) {
On 2012/07/12 22:19:19, epoger wrote:
> I should probably pull Result into its own class file, but I'd like to do that
> in a later CL if that's OK.

Sounds good to me. I built skdiff monolithically C-style because that's what the
other Skia tools did, but it's grown enough to have some engineering applied to
it. At one point Cary did some work in that direction, but I don't believe he
ever committed it.

<bikeshed>When you do that, consider changing these switch statements into
arrays?</bikeshed>

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode1113
tools/skdiff_main.cpp:1113: "\n    --failonresult <result>: exit with nonzero
return code if any file pairs"
Aha! This does not mean "as soon as", but merely "if".
Perhaps "After comparing all files, exit with nonzero..."

[epoger, 2012-07-13 14:19:37]

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp
File tools/skdiff_main.cpp (right):

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode60
tools/skdiff_main.cpp:60: Result getResultByName(const char *name) {
On 2012/07/13 13:02:12, TomH wrote:
> On 2012/07/12 22:19:19, epoger wrote:
> > I should probably pull Result into its own class file, but I'd like to do
that
> > in a later CL if that's OK.
> 
> Sounds good to me. I built skdiff monolithically C-style because that's what
the
> other Skia tools did, but it's grown enough to have some engineering applied
to
> it. At one point Cary did some work in that direction, but I don't believe he
> ever committed it.

Thanks.  I have filed https://code.google.com/p/skia/issues/detail?id=708
('clean up skdiff_main.cpp') to myself to track this.

> <bikeshed>When you do that, consider changing these switch statements into
> arrays?</bikeshed>

Agreed.

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode1113
tools/skdiff_main.cpp:1113: "\n    --failonresult <result>: exit with nonzero
return code if any file pairs"
On 2012/07/13 13:02:12, TomH wrote:
> Aha! This does not mean "as soon as", but merely "if".
> Perhaps "After comparing all files, exit with nonzero..."

Good idea.  I have replaced this with a disturbingly verbose description... I am
open to suggestions for a less verbose, but similarly descriptive, description.
:-)

[TomH, 2012-07-13 14:22:09]

Still LGTM.

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp
File tools/skdiff_main.cpp (right):

https://codereview.appspot.com/6392064/diff/1/tools/skdiff_main.cpp#newcode1113
tools/skdiff_main.cpp:1113: "\n    --failonresult <result>: exit with nonzero
return code if any file pairs"
On 2012/07/13 14:19:37, epoger wrote:
> On 2012/07/13 13:02:12, TomH wrote:
> > Aha! This does not mean "as soon as", but merely "if".
> > Perhaps "After comparing all files, exit with nonzero..."
> 
> Good idea.  I have replaced this with a disturbingly verbose description... I
am
> open to suggestions for a less verbose, but similarly descriptive,
description.
> :-)

I am disturbed by your lack of conciseness, Admiral Poger.

[rmistry, 2012-07-13 18:11:34]

LGTM