770865 - Rename TreeContext as ParseContext

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

njn: You mentioned this would bitrot some work of yours, but by the time I read that I had already made this patch. So how bad is it? I'm sure we can figure something out. I can manually rebase your patch for you, if you like.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Huge, but quite mechanical.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 639079 [details] [diff] [review]
v1

Review of attachment 639079 [details] [diff] [review]:
-----------------------------------------------------------------

It will bit-rot me, but I'll take the hit for the greater good.

I like how you fixed a couple of incorrect comments;  that shows you were paying attention as you made the changes :)  The removal of unnecessary #includes is also good.

I looked at most of the patch closely, but I'll admit to skimming the Parser.cpp changes.

If you haven't already, can you grep for the following patterns to make sure you've changed everything you should:

  TreeContext
  \<tc\>
  TC\>

I think those patterns will catch everything.

If you want a follow-up:  you should be able to move ParseContext into Parser.h and rename the ParseContext module as SharedContext.
(In fact, if you did that before this patch, you'd be able to rename the TreeContext module as SharedContext directly, without calling it ParseContext as an intermediate step.  That's what I was planning to do.)

And then you'll be able to move StmtInfoPC into Parser.h.

Also, since we're clearly on the same wave-length at the moment:  if you're thinking of splitting BytecodeEmitter into BytecodeEmitter and BytecodeContext in order to mirror the Parser/ParseContext split, don't bother.  I tried it and it just made things more complicated;  having three fields that are the same in every BytecodeEmitter is a small price to pay to avoid the split into two classes.  (I even considered combining Parser and ParseContext into a single class, but that was too hard and ugly.)

Comment 3

[Jason Orendorff [:jorendorff]]

This is bitrotted beyond belief. Fallout from the delay caused by bug 770865.

Comment 4

[Luke Wagner [:luke]]

I saw this was backed out due to bustage.  Around the same time, there was a bug 771870 which caused anything to burn that added a file.  Just in case you wanted to reland it b/c boy is ParseContext a better name than TreeContext...

Comment 5

[Nicholas Nethercote [inactive]]

> I saw this was backed out due to bustage.

It never landed, AFAICT, but it would be nice if it did :)

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Rename TreeContext as ParseContext.

Big and mechanical, like your previous patch.  This is applied on top of the
patches in bug 782871.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 652681 [details] [diff] [review]
Rename TreeContext as ParseContext.

Review of attachment 652681 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing with permission.

::: js/src/frontend/Parser-inl.h
@@ +42,5 @@
>      vars_(prs->context),
>      yieldNode(NULL),
>      functionList(NULL),
>      queuedStrictModeError(NULL),
> +    parserPC(&prs->pc),

Feel free to ignore, but I just happened to notice: could we just have ParseContext store a pointer to the Parser instead?  The only use seems to be in ~ParserContext which really just wants to write: "JS_ASSERT(parser->tc == this; parser->tc = parent;".

::: js/src/frontend/Parser.cpp
@@ +3279,5 @@
>      }
>  
>  #if JS_HAS_BLOCK_SCOPE
>      if (blockObj)
> +        PopStatementPC(context, pc);

I think it would be nice if we could have:
  pc->pushStatement / pc->popStatement
This would avoid the name Push/PopStatementPC which makes my brain thing "Program Counter".  (All uses of 'pc' do this a bit, but the name of this one is especially ambiguous-sounding.)  Up to you.

Comment 8

[Nicholas Nethercote [inactive]]

> Feel free to ignore, but I just happened to notice: could we just have
> ParseContext store a pointer to the Parser instead?  The only use seems to
> be in ~ParserContext which really just wants to write: "JS_ASSERT(parser->tc
> == this; parser->tc = parent;".

My motivation in not doing that was to avoid a cyclic dependency between ParseContext and Parser.  Now, because it's just a pointer we could just forward declare Parser before ParseContext, but it still felt nicer to store a ParseContext pointer instead.


> I think it would be nice if we could have:
>   pc->pushStatement / pc->popStatement
> This would avoid the name Push/PopStatementPC which makes my brain thing
> "Program Counter".  (All uses of 'pc' do this a bit, but the name of this
> one is especially ambiguous-sounding.)  Up to you.

I agree about the "PC" ambiguity.  But PushStatementPC and PopStatementPC are both local to Parser.cpp, and moving them into class ParseContext would expose them in ParseContext.h, which isn't great.

Maybe the existing |PushStatement| could become |PushStatementBase| and then |PushStatementPC| and |PushStatementBCE| could both be renamed as |PushStatement|?  The latter two are in separate files, so it wouldn't be too confusing.

Comment 9

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> My motivation in not doing that was to avoid a cyclic dependency between
> ParseContext and Parser.  Now, because it's just a pointer we could just
> forward declare Parser before ParseContext, but it still felt nicer to store
> a ParseContext pointer instead.

I'm not sure if anything is really gained by avoiding the forward decl (the structure is inherently cyclic, regardless of what type names we use).

> I agree about the "PC" ambiguity.  But PushStatementPC and PopStatementPC
> are both local to Parser.cpp, and moving them into class ParseContext would
> expose them in ParseContext.h, which isn't great.

Can we just make ParseContext.h only be #included by Parser.cpp?  I only see it used via pointers in Parser.h.  In fact, could we just move the contents of TreeContext.{h,cpp} into Parser.cpp?  It would avoid what feels like an artificial separation that I started with the new TreeContext::define et al (which I still think was the right thing to do, since it allowed me to encapsulate the declaration state in the right place).

Comment 10

[Luke Wagner [:luke]]

Oh, hah, you just did move TreeContext stuff into Parser.cpp.  Seems like you could move the TreeContext decl from Parser.h into Parser.cpp w/o much trouble and then there would be no concern for putting pushStatement on ParserContext.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73de2885e0e3

Comment 12

[Nicholas Nethercote [inactive]]

> Seems like you could move the TreeContext decl from Parser.h into Parser.cpp
> w/o much trouble

I just tried that but BytecodeCompiler.cpp needs access to ParseContext (formerly TreeContext) and its constructor and destructor :(

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/73de2885e0e3