Please review JDK-8012518

[Hannes Wallnoefer]

Looks good!

Hannes

Am 2014-10-13 um 10:20 schrieb Andreas Gabrielsson:
> Hi,
>
> New webrev at http://cr.openjdk.java.net/~lagergren/8012518.6/. I have 
> fixed the issues you point out.
>
> Regarding the performance testing Marcus already replied with an 
> accurate answer.
>
> Andreas
>
> On 2014-10-09 18:25, Hannes Wallnoefer wrote:
>> 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
>>>>
>>>
>>
>