[webkit-reviews] review denied: [Bug 28243] JavaScriptCore tweaks to get ready for the parser arena : [Attachment 34711] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 28243: JavaScriptCore tweaks to get ready for the parser arena
https://bugs.webkit.org/show_bug.cgi?id=28243

Attachment 34711: patch
https://bugs.webkit.org/attachment.cgi?id=34711&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Several of my comments are suggestions not absolutes.  However, I feel like
there are enough possible changes that it would be good to give it a look over
after you decide what changes to do so r- for now.


> Index: JavaScriptCore/JavaScriptCore.exp

I wonder if you can get rid of this export "__ZN3JSC10JSFunction4infoE" since I
suspect that the code you moved was the only use of JSFunction::info outside of
JavsScriptCore.


> Index: JavaScriptCore/debugger/Debugger.cpp
> +void Debugger::recompileAllJSFunctions(JSGlobalData* globalData, bool
callSourceParsed)

Would be good to make "bool callSourceParsed" an enum.

> Index: JavaScriptCore/parser/Lexer.cpp

> +bool Lexer::scanRegExp(const Identifier*& pattern, const Identifier*& flags,
UChar prefix)

Consider "patternPrefix" instead of "prefix"


> +bool Lexer::skipRegExp()
> +{
> +    bool lastWasEscape = false;
> +    bool inBrackets = false;

> +    while (true) {
> +	   if (isLineTerminator(m_current) || m_current == -1)
> +	       return false;
> +	   if (m_current != '/' || lastWasEscape || inBrackets) {
> +	       // keep track of '[' and ']'
> +	       if (!lastWasEscape) {
> +		   if (m_current == '[' && !inBrackets)
> +		       inBrackets = true;
> +		   if (m_current == ']' && inBrackets)
> +		       inBrackets = false;
> +	       }
> +	       lastWasEscape = !lastWasEscape && m_current == '\\';
> +	   } else { // end of regexp
> +	       shift1();
> +	       break;
> +	   }
> +	   shift1();
> +    }

This is fine as is but it feels like there are unnecessary branches going on
(and it could be slightly tighter by making only one call to shift1();).

Consider this:
     while (true) {
	if (isLineTerminator(m_current) || m_current == -1)
	    return false;

	int current = m_current;
	shift1():
	if (current != '/' || lastWasEscape || inBrackets) {
	    if (!lastWasEscape) {
		switch (current) {
		case '[':
		    if (!inBrackets)
			inBrackets = true;
		    break;
		case ']':
		    if (inBrackets)
			inBrackets = false;
		    break;
		case '\\':
		    lastWasEscape = true;
		    break;
		}
	    } else
		lastWasEscape = false;
	} else
	    break;
    }

I left the if logic following the same order (rather than a break fast
approach) so branch prediction will work the same as before.

And of course, this applies to Lexer::scanRegExp as well.


> Index: JavaScriptCore/profiler/Profiler.cpp
> +	   if (!function->body()->isHostFunction())

It is not obvious to me why function->body() cannot be 0 here. (Previously this
was checked for.)


> Index: JavaScriptCore/runtime/FunctionPrototype.cpp
> +	   FunctionBodyNode* body = function->body();
> +	   if (!body->isHostFunction()) {

It is not obvious to me why function->body() cannot be 0 here. (Previously this
was checked for.)

>  

> Index: JavaScriptCore/runtime/JSFunction.h
>	   static JS_EXPORTDATA const ClassInfo info;

You may not need "JS_EXPORTDATA" here anymore due to moving that code from
WebCore.

> +	   bool isHostFunction() const;

Why not just get rid of this function from the header and just put a non-member
inline function that takes "FunctionBodyNode* body" inside of JSFunction.cpp?
(So bad calls can't be done to this method and you could get rid of the NonLine
postfix on the other one.)


> Index: WebCore/inspector/JavaScriptDebugServer.cpp

 I think you can 
  * get rid of runtime/CollectorHeapIterator.h from the includes in this file.
  * delete WebCore/ForwardingHeaders/runtime/CollectorHeapIterator.h and 
  * get rid of the "private" attribute for the header in
JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj (so that it is a
project header).