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
<[email protected]> wrote:
> Webrev: http://cr.openjdk.java.net/~lagergren/8012518.2/
>
> https://bugs.openjdk.java.net/browse/JDK-8012518
