[webkit-reviews] review denied: [Bug 21281] Some parser cleanup : [Attachment 23985] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 's request for review:
Bug 21281: Some parser cleanup
https://bugs.webkit.org/show_bug.cgi?id=21281

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.