Treat default command line argument properly.

Treat default command line argument properly.

In SkCommandLineFlags, if the client sets a default value
of multiple arguments (e.g. "arg0 arg1 ..."), set
the actual defaults to all of those arguments separately
(i.e. an array with [0] == "arg0", [1] == "arg1", ...),
rather than as one string (i.e. [0] == "arg0 arg1 ...").

Remove the hack that worked around this bug.

Also move the increasingly complicated implementation of
SkFlagInfo::CreateStringFlag into the cpp file.

BUG=https://code.google.com/p/skia/issues/detail?id=1237

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

[epoger, 2013-04-23 16:54:13]

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
File tools/flags/SkCommandLineFlags.cpp (right):

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.cpp:14: SkFlagInfo* info = SkNEW_ARGS(SkFlagInfo,
(name, shortName, kString_FlagType, helpString));
Is this ever freed?  Does it matter?

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.cpp:17: info->fStrings = pStrings;
Why is it important for the strings to be stored in the StringArray object
pointed to by pStrings, rather than SkFlagInfo just allocating its own
StringArray object?

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.cpp:25: while (begin < defaultEnd && ' ' ==
*begin) {
I think it would be cleaner to write a helper function that converts a
space-delimited string to a StringArray, and call that here.

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlags.h
File tools/flags/SkCommandLineFlags.h (right):

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.h:69: *  creates an array:
Thoughts for a separate CL: I think it would be cleaner for DEFINE_string to
specify a *single* string value, like how DEFINE_int32 specifies a single int32
value.  Any spaces within the string value would be preserved.

Then you could create a separate DEFINE_string_array() that acts like what we
have now.

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.h:232: static bool CreateStringFlag(const char*
name, const char* shortName,
Please add documentation for this (and maybe other?) functions.  Specifically,
it took me some digging to understand that pStrings is passed in just so we have
a place to store strings (whatever strings were stored there before this call
will be nuked by a reset() within this call)

[scroggo, 2013-04-24 18:42:35]

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
File tools/flags/SkCommandLineFlags.cpp (right):

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.cpp:14: SkFlagInfo* info = SkNEW_ARGS(SkFlagInfo,
(name, shortName, kString_FlagType, helpString));
On 2013/04/23 16:54:13, epoger wrote:
> Is this ever freed?  Does it matter?

Yes. It is freed at
https://code.google.com/p/skia/source/browse/trunk/tools/flags/SkCommandLineF...
so long as the caller calls Parse.

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.cpp:17: info->fStrings = pStrings;
On 2013/04/23 16:54:13, epoger wrote:
> Why is it important for the strings to be stored in the StringArray object
> pointed to by pStrings, rather than SkFlagInfo just allocating its own
> StringArray object?

Because pStrings is what the client will actually use to access the arguments
after parsing (when the SkFlagInfo will be deleted).

So if you wrote:

DEFINE_string(ignoreErrorTypes, ...)

the macro creates an array called FLAGS_ignoreErrorTypes with global scope (in
that file), which you can then access to determine what settings the user
supplied on the command line.

I've added some comments that I hope make that clearer.

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.cpp:25: while (begin < defaultEnd && ' ' ==
*begin) {
On 2013/04/23 16:54:13, epoger wrote:
> I think it would be cleaner to write a helper function that converts a
> space-delimited string to a StringArray, and call that here.

I like that approach too, except that I also like making StringArray's
modification functions being private because it means the client won't
accidentally modify them. I compromised by making the helper function a member
function of SkFlagInfo.

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlags.h
File tools/flags/SkCommandLineFlags.h (right):

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.h:69: *  creates an array:
On 2013/04/23 16:54:13, epoger wrote:
> Thoughts for a separate CL: I think it would be cleaner for DEFINE_string to
> specify a *single* string value, like how DEFINE_int32 specifies a single
int32
> value.  Any spaces within the string value would be preserved.
> 
> Then you could create a separate DEFINE_string_array() that acts like what we
> have now.

Good idea. I have filed https://code.google.com/p/skia/issues/detail?id=1262

https://codereview.chromium.org/14366034/diff/1/tools/flags/SkCommandLineFlag...
tools/flags/SkCommandLineFlags.h:232: static bool CreateStringFlag(const char*
name, const char* shortName,
On 2013/04/23 16:54:13, epoger wrote:
> Please add documentation for this (and maybe other?) functions.  Specifically,
> it took me some digging to understand that pStrings is passed in just so we
have
> a place to store strings (whatever strings were stored there before this call
> will be nuked by a reset() within this call)

Done.

[epoger, 2013-04-24 19:12:54]

LGTM

Thanks for adding the comments!

https://codereview.chromium.org/14366034/diff/6001/tools/flags/SkCommandLineF...
File tools/flags/SkCommandLineFlags.h (right):

https://codereview.chromium.org/14366034/diff/6001/tools/flags/SkCommandLineF...
tools/flags/SkCommandLineFlags.h:232: *  @param pType Pointer to a global
variable which holds the value set by SkCommandLineFlags.
maybe instead of pType, call this p<Type>?  (in the documentation)

[scroggo, 2013-04-24 19:23:47]

https://codereview.chromium.org/14366034/diff/6001/tools/flags/SkCommandLineF...
File tools/flags/SkCommandLineFlags.h (right):

https://codereview.chromium.org/14366034/diff/6001/tools/flags/SkCommandLineF...
tools/flags/SkCommandLineFlags.h:232: *  @param pType Pointer to a global
variable which holds the value set by SkCommandLineFlags.
On 2013/04/24 19:12:54, epoger wrote:
> maybe instead of pType, call this p<Type>?  (in the documentation)

Agreed. That sounds much better.

[scroggo, 2013-04-24 19:25:32]

Committed patchset #3 manually as r8845 (presubmit successful).