Please review JDK-8012518

[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
>>>
>>
>