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