Please review JDK-8012518

[Attila Szegedi]

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