[webkit-reviews] review denied: [Bug 131004] Add a --functionsToDFGCompile option. : [Attachment 228286] patch 3: removed unnecessary white space checks

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 131004: Add a --functionsToDFGCompile option.
https://bugs.webkit.org/show_bug.cgi?id=131004

Attachment 228286: patch 3: removed unnecessary white space checks
https://bugs.webkit.org/attachment.cgi?id=228286&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=228286&action=review


This feature isn't very useful to me, for a few reasons:

(1) It relies on function names, but not all functions have names, and names
can overlap. This would be much more useful to me if its input were
"filename:line,column" instead.

(2) It doesn't produce the initial exhaustive file, that I would start cutting
down.

(3) There are enough special modes here -- three kinds of comments (//, /* */,
and certain combinations of "non-function" characters), a special header
delimiter if you're going to supply a file, all text assumed to be ASCII,
limited range of parseable characters -- that I'm probably going to have to
re-read the parsers every time I use this feature, and the parsers aren't super
readable.

Now, maybe I should say r+ because you used this feature once and you like it,
and it's only debugging code anyway. But I think the bar needs to be higher for
code that we agree to maintain together.

I'm not convinced when you list out all the ways you could use all the features
here. Every feature can be used in some circumstance, but that doesn't make
every feature good, nor does it make all implementations of the same feature
equally good. We need to weigh a feature against other ways to accomplish the
same task, and against simplicity, clarity, maintainability, and robustness.

> Source/JavaScriptCore/runtime/Options.cpp:85
> +static bool parse(const char* string, const char*& value)
> +{
> +    value = string;
> +    return true;
> +}

This function returns bool but never returns false. Confusing.

> Source/JavaScriptCore/runtime/Options.cpp:352
> +struct FunctionsFilter {

This should be a class.

> Source/JavaScriptCore/runtime/Options.cpp:359
> +    Vector<CString> functionNames;

This should be Vector<String>.

> Source/JavaScriptCore/runtime/Options.cpp:362
> +static inline bool isFunctionNameStart(char c)

This is not a correct definition of the start of a function name in JavaScript.
Many important details are missing, including escapes and Unicode.

> Source/JavaScriptCore/runtime/Options.cpp:367
> +static inline bool isFunctionNameChar(char c)

Ditto.

> Source/JavaScriptCore/runtime/Options.cpp:376
> +    char c = *p;
> +    while (c) {

It is better for a parser to operate on a set of characters and a length.
In-band signaling of end-of-data often produces bugs.

> Source/JavaScriptCore/runtime/Options.cpp:378
> +	   // Skip // or /* */ comment entries:
> +	   if (c && c == '/') {

If you factored this out into a helper function, you wouldn't need a comment to
give it a name.

> Source/JavaScriptCore/runtime/Options.cpp:381
> +		   continue;

continue is misleading / slow here. You want break.

> Source/JavaScriptCore/runtime/Options.cpp:386
> +

Extra newline.

You should put some explicit control flow here to make it easier to follow.

> Source/JavaScriptCore/runtime/Options.cpp:432
> +    std::unique_ptr<char[]> buffer(new char[size + 1]);

This should be "auto buffer = make_unique<>...".

> Source/JavaScriptCore/runtime/Options.cpp:459
> +    if (filters.isInitialized && !filters.functionNames.size())
> +	   return true;
> +
> +    if (!filters.isInitialized) {
> +	   if (!strncmp(filterOption, "file:", 5))
> +	       parseFunctionNamesInFile(filters, &filterOption[5]);
> +	   else
> +	       parseFunctionNamesInString(filters, filterOption);
> +	   filters.isInitialized = true;
> +    }

Lazy initialization should be built into the class.