Please review JDK-8012518

[Hannes Wallnoefer]

Hi Andreas,

This looks good. Some minor javadoc nitpicks:

  - First and last @param for ParserContextFunctionNode constructor do 
not exist
  - Your @param tags often miss a description
  - I think the proper way to link to an enum value is #VALUE, e.g. 
{@link CompilationState#PARSED} in FunctionNode

Apart from these minor issues the patch looks good.

Out of curiosity, what scripts did you use for performance testing? 
Mandreel might be interesting.

Hannes

Am 2014-10-08 um 11:13 schrieb Andreas Gabrielsson:
> Now all of Attila's comments should be fixed. I have also removed some 
> public setters that were not needed anymore.
>
> Me and Marcus run a few performance tests and they passed, there were 
> basically no difference.
>
> New webrev is at: http://cr.openjdk.java.net/~lagergren/8012518.5/
>
> Andreas
>
> On 2014-09-16 16:06, Andreas Gabrielsson wrote:
>> I have fixed the thing you list below, thank you.
>> New webrev uploaded at: http://cr.openjdk.java.net/~lagergren/8012518.3/
>> On 2014-09-16 10:50, Attila Szegedi wrote:
>>> .hgignore:
>>> -what's OpenJDK policy for these? I see we have .idea/* in there; 
>>> how about we put .externalToolBuilders/* and .settings/* there instead?
>>>
>>> Block.java:
>>> - I don't like variables named "tmp" anything. "terminalFlags"?
>>> - New public constructors are entirely undocumented. Not that the 
>>> old one had quality documentation, but still…
>>> - private constructor no longer sets finish. Why is it still in the 
>>> parameter list?
>>>
>>> ForNode.java:
>>> - undocumented public constructor
>>>
>>> LoopNode.java:
>>> - undocumented protected constructor parameters
>>>
>>> WhileNode.java
>>> - undocumented public constructor parameters
>>>
>>> In all the new classes for parser context implementation:
>>> - add empty line between package declaration and imports
>>> - the import ordering doesn't follow our source code policy of 
>>> "strict alphabetical, no grouping separator lines"
>>> - do these classes need to be public?
>>>
>>> Parser.java:
>>> - many local variables and method parameters could be declared final.
>>> - I generally dislike assigning an expression only used once to a 
>>> local variable, e.g.
>>>      List<Statement> statements = newBlock.getStatements();
>>>      return new Block(blockToken, finish, newBlock.getFlags(), 
>>> statements);
>>> or
>>>      FunctionNode scriptFunction = createFunctionNode(script, 
>>> functionToken, ident, new ArrayList<IdentNode>(), 
>>> FunctionNode.Kind.SCRIPT, functionLine, programBody);
>>>      return scriptFunction;
>>> and there are more.
>>>
>>> BaseParserContextNode.java:
>>> - several cases of missing space before opening curly brace
>>> - missing "final"s on function parameters
>>> - missing documentation
>>>
>>> ParserContext.java
>>> - getLastStatement(): consider extracting common subexpression 
>>> stack[sp - 1].getStatements() into a local variable
>>> - if you made push() generic as  public <T extends 
>>> ParserContextNode> T push(final T node) then you wouldn't need 
>>> casting its return values in Parser.
>>>
>>> ParserContextFunctionNode.java
>>> - consider extracting the (flags & FunctionNode.SOME_FLAG) != 0 
>>> pattern into a boolean getFlag(final int flag) function
>>>
>>> Most of these remarks are related to stylistic issues: we try to 
>>> ensure everything has a JavaDoc, we're strict about using the 
>>> "final" keyword everywhere possible (you can have an Eclipse save 
>>> action that adds them), we have a set policy for organizing imports 
>>> so that different people's IDE settings don't cause unnecessary 
>>> noise in diffs, etc. You'll get the hang of it as you go. Nice work 
>>> overall!
>>>
>>> Attila.
>>>
>>> On Sep 15, 2014, at 5:01 PM, Andreas Gabrielsson 
>>> <andreas.gabrielsson at oracle.com> wrote:
>>>
>>>> Webrev: http://cr.openjdk.java.net/~lagergren/8012518.2/
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8012518
>>
>