All looks good to me (with Hannes' remarks). +1. On Oct 8, 2014, at 11:13 AM, Andreas Gabrielsson <[email protected]> wrote:
> 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 >> >
