refactor duplication (shouldSkip and skip_name) into a utility function

refactor duplication (shouldSkip and skip_name) into a utility function

R=caryclark@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=10280

Committed: https://code.google.com/p/skia/source/detail?r=10317

[caryclark, 2013-07-22 20:36:36]

lgtm, but let reed weigh in before committing

[reed1, 2013-07-23 14:01:40]

https://codereview.chromium.org/19807005/diff/2001/tools/ToolUtils.h
File tools/ToolUtils.h (right):

https://codereview.chromium.org/19807005/diff/2001/tools/ToolUtils.h#newcode13
tools/ToolUtils.h:13: bool shouldSkip(const SkTDArray<const char*>& strings,
const char* name);
1. This file name is slightly odd, in that it has no Sk
2. This function is poorly scoped: no sk, no class wrapper
3. Is there really no existing .h we can add this one function to?

[sglez, 2013-07-23 15:16:20]

On 2013/07/23 14:01:40, reed1 wrote:
> https://codereview.chromium.org/19807005/diff/2001/tools/ToolUtils.h
> File tools/ToolUtils.h (right):
> 
> https://codereview.chromium.org/19807005/diff/2001/tools/ToolUtils.h#newcode13
> tools/ToolUtils.h:13: bool shouldSkip(const SkTDArray<const char*>& strings,
> const char* name);
> 1. This file name is slightly odd, in that it has no Sk
> 2. This function is poorly scoped: no sk, no class wrapper
> 3. Is there really no existing .h we can add this one function to?

The #1 candidate was SkCommandLineFlags, because this function is mostly used to
parse flags.
The problem with that is benchmain.cpp is using the function, and it doesn't
seem right to make bench depend on flags just because of this function.

Candidate #2 was core/SkStringUtils.h which was discarded because, being part of
skialib, there should not be code there that is only used for internal tools.

Candidate #3 was SkParse.h/cpp.. And I don't think we discarded that one... 
It is old code that is using c-style (T* array, int length) instead of
SkTDArray, so this function would look odd in there. 

What do you think? Add it to SkParse? If so, change the signature of the
function to the old-school style of the file or keep it like it is?

[reed1, 2013-07-23 15:18:59]

I think I'd vote for SkCommandLineFlags for now, since this new function is only
needed by tools, and need not creep into our src/ tree yet.

[sglez, 2013-07-23 15:40:42]

Function is now a static method in SkCommandLineFlags

[reed1, 2013-07-23 15:41:32]

https://codereview.chromium.org/19807005/diff/15001/tools/flags/SkCommandLine...
File tools/flags/SkCommandLineFlags.h (right):

https://codereview.chromium.org/19807005/diff/15001/tools/flags/SkCommandLine...
tools/flags/SkCommandLineFlags.h:118: static bool shouldSkip(const
SkTDArray<const char*>& strings, const char* name);
static methods are Capitalized (see Parse above)

[sglez, 2013-07-23 15:45:53]

On 2013/07/23 15:41:32, reed1 wrote:
>
https://codereview.chromium.org/19807005/diff/15001/tools/flags/SkCommandLine...
> File tools/flags/SkCommandLineFlags.h (right):
> 
>
https://codereview.chromium.org/19807005/diff/15001/tools/flags/SkCommandLine...
> tools/flags/SkCommandLineFlags.h:118: static bool shouldSkip(const
> SkTDArray<const char*>& strings, const char* name);
> static methods are Capitalized (see Parse above)

Sorry about that... Capitalized

[reed1, 2013-07-23 15:47:08]

lgtm

[sglez, 2013-07-23 17:26:38]

Committed patchset #4 manually as r10280 (presubmit successful).

[sglez, 2013-07-23 22:17:43]

One line change.

This change got reverted because I removed the SK_FORCE_LINKING line. I
mistakenly thought it wasn't needed and broke some of the gm tests.

[sglez, 2013-07-24 14:39:15]

Requesting blessing for one-line fix to this reverted change

[reed1, 2013-07-24 15:01:05]

lgtm

[sglez, 2013-07-24 17:24:27]

Committed patchset #5 manually as r10317 (presubmit successful).