Looks good!
Hannes
Am 2014-10-13 um 10:20 schrieb Andreas Gabrielsson:
Hi,
New webrev at http://cr.openjdk.java.net/~lagergren/8012518.6/. I have
fixed the issues you point out.
Regarding the performance testing Marcus already replied with an
accurate answer.
Andreas
On 2014-10-09 18:25, Hannes Wallnoefer wrote:
Hi Andreas,
This looks good. Some minor javadoc nitpicks:
- First and last @param for ParserContextFunctionNode constructor do
not exist
- Your @param tags often miss a description
- I think the proper way to link to an enum value is #VALUE, e.g.
{@link CompilationState#PARSED} in FunctionNode
Apart from these minor issues the patch looks good.
Out of curiosity, what scripts did you use for performance testing?
Mandreel might be interesting.
Hannes
Am 2014-10-08 um 11:13 schrieb Andreas Gabrielsson:
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
