Bug 21281 – Some parser cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Geoffrey Garen]

Patch coming.

[Geoffrey Garen]

Created attachment 23985 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 23985 [details]
patch

So i read through this.  It looked fine.  If all you need is a rubber-stamp, here's my stamp.  If you need a more thorough review...

[Darin Adler]

Comment on attachment 23985 [details]
patch

The change here is really great; I love the improvement in both clarity and structure.

I'm not a fond of the word "Range" in SourceRange. It makes it sound like the class is used to keep track of various arbitrary ranges in an overall piece of source code, but it doesn't seem to me that's what it's for at all. SourceProvider doesn't know anything about line numbers, yet SourceRange does; that's actually one of the biggest differences between the two.

Maybe the name SourceCode would be better than SourceRange. Even though the class is capable of representing different ranges within a single source provider, I don't think it needs to be named after this capability.

Lexer::setCode is one of the functions that makes me think this could be a good idea.

+ int asId() { return reinterpret_cast<intptr_t>(this); }

Why do IDs need to be integers? This code above is really weak, for example an int is smaller than an intptr_t in 64-bit, so this code chops off high bits.

+ void setCode(const SourceRange*);

Why a pointer rather than a reference? I'm sure there's a good reason.

You can hand off the storage from one Vector to another by using swap. Not sure if that's relevant.

Why is ScopeNode::m_sourceRange protected rather than private?

+ OwnPtr<Identifier> m_parameters;

I think this needs to be an OwnArrayPtr.

I think you should consider some syntactic sugar to make it easier to create a SourceRange directly from without explicitly having to create the appropriate type of source provider. To me, the lines of code that construct SourceRange objects seem kind of long and it seems to come up over and over again.

review- because of the OwnPtr and chopping off high bits of 64-bit pointers issues.

[Geoffrey Garen]

> The change here is really great; I love the improvement in both clarity and
> structure.

Thank you! :)

> I'm not a fond of the word "Range" in SourceRange. It makes it sound like the
> class is used to keep track of various arbitrary ranges in an overall piece of
> source code, but it doesn't seem to me that's what it's for at all.
> SourceProvider doesn't know anything about line numbers, yet SourceRange does;
> that's actually one of the biggest differences between the two.
> 
> Maybe the name SourceCode would be better than SourceRange. Even though the
> class is capable of representing different ranges within a single source
> provider, I don't think it needs to be named after this capability.
> 
> Lexer::setCode is one of the functions that makes me think this could be a good
> idea.

Renamed to SourceCode.

> + int asId() { return reinterpret_cast<intptr_t>(this); }
> 
> Why do IDs need to be integers? This code above is really weak, for example an
> int is smaller than an intptr_t in 64-bit, so this code chops off high bits.

Fixed for everything except WebScriptDebugDelegate, for which I've filed a bug report with Apple's DashCode team.

> + void setCode(const SourceRange*);
> 
> Why a pointer rather than a reference? I'm sure there's a good reason.

Changed to a reference. I was using a pointer because the class ultimately wanted to store a pointer, but I changed my mind when I realized that SourceRange was a reference everywhere else.

> You can hand off the storage from one Vector to another by using swap. Not sure
> if that's relevant.

Ha! I was looking for that, but got confused by my assumption that it would be named adopt. Anyway, releaseBuffer() is a little better because it saves a little space in FunctionBodyNode not to have a Vector.

> Why is ScopeNode::m_sourceRange protected rather than private?

Made private.

> + OwnPtr<Identifier> m_parameters;
> 
> I think this needs to be an OwnArrayPtr.

Changed to manually call fastFree. OwnArrayPtr is no good because it's not allocated by new[].

> I think you should consider some syntactic sugar to make it easier to create a
> SourceRange directly from without explicitly having to create the appropriate
> type of source provider. To me, the lines of code that construct SourceRange
> objects seem kind of long and it seems to come up over and over again.

Added a makeSource function in JSC and WebCore.

[Geoffrey Garen]

Created attachment 24003 [details]
patch

[Cameron Zwarich (cpst)]

Comment on attachment 24003 [details]
patch

In the ChangeLog, you say wtf:: instead of WTF::.

You use both sourceID and sourceId in your patch. I know the latter is because the original code had it, but you should probably pick one and stick with it. Since sourceID is our coding style, that seems like a good choice.

I will say r=me, but due to Darin's earlier review, I would like to see this looked over by him after it lands.

[Geoffrey Garen]

Committed revision 37184.

[Darin Adler]

Comment on attachment 24003 [details]
patch

+    if (m_parameters)
+        fastFree(m_parameters);

fastFree already checks for 0, so that's not needed. But also, this doesn't seem to destroy the Identifier objects in m_parameters.