Please review JDK-8012518

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