Please review JDK-8012518

[Andreas Gabrielsson]

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