Improve abstraction of parse state #221

jvdb · 2017-10-26T07:27:49Z

Resolves #216.

This PR tackles some abstraction issues. The resolution resolves a functional bug. The six commits show the following six changes:

Callbacks was extracted from the Environment class.
The Environment class was renamed to ParseState.
Of the three remaining constructors on ParseState, two are renamed to factory methods to better reflect their functionality.
Both equals() and hashCode() methods were implemented on ParseState.
The eval() methods of ValueExpression and Expression were modified to accept a ParseState instead of a ParseGraph.
The Shorthand approximate implementation of CURRENT_OFFSET is replaced with an implementation that returns the actual offset field of the ParseState.

After that, another branch was merged that recreates an Environment class: #222.

…a ParseState argument instead of a ParseGraph argument.

…'s offset field.

codecov · 2017-10-26T07:27:52Z

Codecov Report

Merging #221 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master   #221   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity      952    967   +15     
=======================================
  Files            86     88    +2     
  Lines          1314   1331   +17     
  Branches        134    137    +3     
=======================================
+ Hits           1314   1331   +17

Impacted Files	Coverage Δ	Complexity Δ
...ingdata/metal/expression/value/arithmetic/Mod.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...singdata/metal/expression/value/reference/Len.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...arsingdata/metal/expression/value/bitwise/Not.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ava/io/parsingdata/metal/expression/value/Cat.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ata/metal/expression/value/bitwise/ShiftRight.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ingdata/metal/expression/value/arithmetic/Mul.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ingdata/metal/expression/value/arithmetic/Div.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...arsingdata/metal/expression/value/bitwise/And.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ingdata/metal/expression/value/arithmetic/Neg.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...data/metal/expression/value/bitwise/ShiftLeft.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8cdea5...b5f995f. Read the comment docs.

…, Callbacks and Encoding values.

mvanaken

Nice!

mvanaken · 2017-10-30T16:50:04Z

core/src/test/java/io/parsingdata/metal/expression/value/FoldEdgeCaseTest.java

-                return ImmutableList.create(Optional.<Value>empty()).add(Optional.of(new Value(createFromBytes(new byte[] { 1, 2 }), enc())));
-            }
-        }, Shorthand::add).eval(stream(0).order, enc()).isEmpty());
+        assertTrue(foldRight((parseState, encoding) -> ImmutableList.create(Optional.<Value>empty()).add(Optional.of(new Value(createFromBytes(new byte[] { 1, 2 }), enc()))), Shorthand::add).eval(stream(0), enc()).isEmpty());


This is one big line of code. ;)

mvanaken · 2017-10-30T16:55:40Z

core/src/test/java/io/parsingdata/metal/token/TieTest.java

-    private Optional<Environment> checkFullParse(Token token, byte[] data) throws IOException {
-        final Optional<Environment> result = token.parse(new Environment(new InMemoryByteStream(data)), enc());
+    private Optional<ParseState> checkFullParse(Token token, byte[] data) throws IOException {
+        final Optional<ParseState> result = token.parse(createFromByteStream(new InMemoryByteStream(data)), enc());


You can use the ParseStateFactory.stream(bytes) for this.

stream() takes a vararg of integers, not an array :(

mvanaken · 2017-10-30T17:03:49Z

core/src/test/java/io/parsingdata/metal/ErrorsTest.java

@@ -73,15 +73,15 @@ public void multiValueInRepN() throws IOException {
                any("b"),
                repn(dummy, ref("b"))
            );
-       Optional<Environment> result = multiRepN.parse(stream(2, 2, 2, 2), enc());
+       Optional<ParseState> result = multiRepN.parse(stream(2, 2, 2, 2), enc());


Minor, but result can be made final.

mvanaken · 2017-10-30T17:07:00Z

core/src/main/java/io/parsingdata/metal/token/Tie.java

        if (values.isEmpty()) {
-            return complete(() -> success(new Environment(environment.closeBranch().order, returnEnvironment.source, returnEnvironment.offset, returnEnvironment.callbacks)));
+            return complete(() -> success(new ParseState(parseState.closeBranch().order, returnParseState.source, returnParseState.offset)));


Not using a factory method here?

I suppose that would have to be a method comparable to seek() that replaces the ParseGraph on returnEnvironment. It could be done, but this seems to be a rather isolated use.

mvanaken · 2017-10-30T17:09:47Z

core/src/main/java/io/parsingdata/metal/data/ParseState.java

+    public final BigInteger offset;
+    public final Source source;
+
+    public ParseState(final ParseGraph order, final Source source, final BigInteger offset) {


If it is possible you may consider making this ParseState constructor private and always use the ParseStateFactory instead.

As discussed, ParseStateFactory is a test class. I think this single constructor makes sense because it only stores fields.

mvanaken · 2017-10-30T17:11:11Z

core/src/main/java/io/parsingdata/metal/data/ParseState.java

+        this.offset = checkNotNegative(offset, "offset");
+    }
+
+    public static ParseState createFromByteStream(final ByteStream input, final BigInteger offset) {


Shouldn't this method be part of the ParseStateFactory?

See above :)

ccreeten · 2017-10-31T19:05:33Z

core/src/main/java/io/parsingdata/metal/data/DataExpressionSource.java

            && Objects.equals(encoding, ((DataExpressionSource)obj).encoding);
    }

    @Override
    public int hashCode() {
-        return Objects.hash(getClass().hashCode(), dataExpression, index, graph, encoding);
+        return Objects.hash(getClass().hashCode(), dataExpression, index, parseState, encoding);


You can probably remove the calls to hashcode everywhere where using Objects#hash?

It seems to get called on it anyway and all tests are green. Good catch!

ccreeten · 2017-10-31T19:06:06Z

core/src/main/java/io/parsingdata/metal/Shorthand.java

@@ -86,7 +87,7 @@

    public static final Token EMPTY = def(EMPTY_NAME, 0L);
    public static final ValueExpression SELF = new Self();
-    public static final ValueExpression CURRENT_OFFSET = elvis(add(offset(SELF), len(SELF)), con(0));
+    public static final ValueExpression CURRENT_OFFSET = new CurrentOffset();


It's back with a vengeance!

ccreeten · 2017-10-31T19:08:33Z

core/src/main/java/io/parsingdata/metal/data/ParseState.java

+import io.parsingdata.metal.expression.value.ValueExpression;
+import io.parsingdata.metal.token.Token;
+
+public class ParseState {


Perhaps make final?

It's probably worth considering in general which classes should be made final: #223.

ccreeten · 2017-10-31T19:15:41Z

core/src/main/java/io/parsingdata/metal/data/ParseState.java

+import io.parsingdata.metal.expression.value.ValueExpression;
+import io.parsingdata.metal.token.Token;
+
+public class ParseState {


Perhaps add some documentation 😈

.

@mvanaken

…Environment.extendScope() method, based on PR feedback by @mvanaken.

Recreated an Environment class to hold common values during parsing.

…ccreeten

…ccreeten.

jvdb added 6 commits October 24, 2017 11:37

Extracted Callbacks from Environment.

038d69d

Renamed Environment to ParseState.

3cb3eec

Refactored two of the ParseState constructors to factory methods.

3291120

Implemented equals() and hashCode() on ParseState with test coverage.

0e71234

Migrated the eval() method of ValueExpression and Expression to have …

a9f33ad

…a ParseState argument instead of a ParseGraph argument.

#216: Added new CurrentOffset implementation that uses the ParseState…

c01c25f

…'s offset field.

jvdb requested review from TheUnknownCylon, ccreeten and mvanaken October 26, 2017 07:27

jvdb added 2 commits October 27, 2017 14:29

Created a new Environment class, as a tuple to hold scope, ParseState…

4a8488e

…, Callbacks and Encoding values.

Removed unused code and fixed some comment alignments.

27d5953

mvanaken approved these changes Oct 30, 2017

View reviewed changes

ccreeten approved these changes Oct 31, 2017

View reviewed changes

jvdb added 5 commits November 5, 2017 17:38

Added addBranch() to Environment based on PR feedback by @mvanaken.

727e170

Many of Until's method arguments were not final. PR comment by @mvanaken

74eb574

.

Refactored Token.makeScope() and Environment.withScope() to a single …

426d8a4

…Environment.extendScope() method, based on PR feedback by @mvanaken.

Merge pull request #222 from parsingdata/return-of-the-environment

f191858

Recreated an Environment class to hold common values during parsing.

#216: Removed unnecessary call to hashCode() based on PR feedback by @…

b5f995f

…ccreeten.

jvdb merged commit ec4782a into master Nov 5, 2017

jvdb deleted the environment-to-parsestate branch November 5, 2017 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve abstraction of parse state #221

Improve abstraction of parse state #221

jvdb commented Oct 26, 2017 •

edited

Loading

codecov bot commented Oct 26, 2017 •

edited

Loading

mvanaken left a comment

mvanaken Oct 30, 2017

mvanaken Oct 30, 2017

jvdb Nov 5, 2017 •

edited

Loading

mvanaken Oct 30, 2017

mvanaken Oct 30, 2017

jvdb Nov 5, 2017

mvanaken Oct 30, 2017

jvdb Nov 5, 2017

mvanaken Oct 30, 2017

jvdb Nov 5, 2017

ccreeten Oct 31, 2017

jvdb Nov 5, 2017

ccreeten Oct 31, 2017

ccreeten Oct 31, 2017

jvdb Nov 5, 2017

ccreeten Oct 31, 2017

Improve abstraction of parse state #221

Improve abstraction of parse state #221

Conversation

jvdb commented Oct 26, 2017 • edited Loading

codecov bot commented Oct 26, 2017 • edited Loading

Codecov Report

mvanaken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvdb Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvdb commented Oct 26, 2017 •

edited

Loading

codecov bot commented Oct 26, 2017 •

edited

Loading

jvdb Nov 5, 2017 •

edited

Loading