Please review JDK-8012518
attila.szegedi at oracle.com
Tue Sep 16 08:50:50 UTC 2014
-what's OpenJDK policy for these? I see we have .idea/* in there; how about we put .externalToolBuilders/* and .settings/* there instead?
- 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?
- undocumented public constructor
- undocumented protected constructor parameters
- 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?
- 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);
FunctionNode scriptFunction = createFunctionNode(script, functionToken, ident, new ArrayList<IdentNode>(), FunctionNode.Kind.SCRIPT, functionLine, programBody);
and there are more.
- several cases of missing space before opening curly brace
- missing "final"s on function parameters
- missing documentation
- 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.
- 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!
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/
More information about the nashorn-dev