Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Current iteration #242

Merged
merged 26 commits into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e725c68
#236: Basic, compiling and failing tests for the CurrentIndex ValueEx…
jvdb Jul 2, 2018
e6b2f37
#236: Added test for another Token that needs to be included: While.
jvdb Jul 3, 2018
0e12551
#236: Basic implementation and tests.
jvdb Jul 4, 2018
dac4afb
#236: Added (failing) test for using CurrentIndex in a nested iterable.
jvdb Jul 4, 2018
b449cff
#236: Improved currentIndex implementation by looking for the deepest…
mvanaken Jul 17, 2018
a294916
#236: Renamed CurrentIndex to CurrentIteration to better reflect what…
jvdb Jul 18, 2018
ec291c2
#236: Fixed one of the CurrentIteration tests.
jvdb Jul 18, 2018
c3c961e
#236: Simplified implementation of CurrentIteration, which also impro…
jvdb Jul 18, 2018
cab9c3e
#236: Improved test coverage for CurrentIteration.
jvdb Jul 18, 2018
3450821
#236: Converted CurrentIteration to use trampolines for recursion.
jvdb Jul 18, 2018
0d2401b
#236: Added unit test for CurrentIteration.
jvdb Jul 18, 2018
ad98634
#236: Keep track of the last iterable instead of the last iteration a…
mvanaken Jul 20, 2018
f8f5034
#236: Small refactorings.
jvdb Jul 21, 2018
a17967c
#236: Make CurrentIteration return empty value when referencing outsi…
mvanaken Jul 24, 2018
b2961aa
#236: More tests. Unfortunately, no way to more precisely check for e…
jvdb Jul 31, 2018
6fa43c5
#236: New implementation for current iteration, which is now tracked …
mvanaken Aug 20, 2018
ebfcf71
#236: Removed redundant import.
mvanaken Aug 20, 2018
6b72849
#236: Processed review comments of @rdvdijk.
mvanaken Aug 24, 2018
fe71ac2
#236: Included ToString test for ParseState with iterations.
mvanaken Aug 24, 2018
8f9d2c3
#236: Processed review comments from @rdvdijk @ccreeten.
mvanaken Sep 3, 2018
97a66c5
#236: Fixed a failing test.
jvdb Sep 3, 2018
5442e7a
#236: Added test to repair test coverage.
jvdb Sep 10, 2018
93716c3
236: Improved CurrentIterationTest.
jvdb Sep 15, 2018
fd803d8
236: Improved CurrentIterationTest to actually verify zero-sized Def …
jvdb Sep 15, 2018
c63f84c
#236: Replaced null-check with isEmpty()-call, added newline, improve…
jvdb Sep 16, 2018
840c354
#236: Removed dependency on the javafx Pair class by adding trivial I…
jvdb Sep 16, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/src/main/java/io/parsingdata/metal/Shorthand.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import io.parsingdata.metal.expression.value.Cat;
import io.parsingdata.metal.expression.value.Const;
import io.parsingdata.metal.expression.value.ConstantFactory;
import io.parsingdata.metal.expression.value.reference.CurrentIteration;
import io.parsingdata.metal.expression.value.Elvis;
import io.parsingdata.metal.expression.value.Expand;
import io.parsingdata.metal.expression.value.FoldLeft;
Expand Down Expand Up @@ -89,6 +90,7 @@ public final class Shorthand {
public static final Token EMPTY = def(EMPTY_NAME, 0L);
public static final ValueExpression SELF = new Self();
public static final ValueExpression CURRENT_OFFSET = new CurrentOffset();
public static final ValueExpression CURRENT_ITERATION = new CurrentIteration();
public static final Expression TRUE = new True();

private Shorthand() {}
Expand Down
39 changes: 27 additions & 12 deletions core/src/main/java/io/parsingdata/metal/data/ParseState.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.parsingdata.metal.data;

import static java.math.BigInteger.ONE;
import static java.math.BigInteger.ZERO;

import static io.parsingdata.metal.Util.checkNotNegative;
Expand All @@ -36,43 +37,55 @@ public class ParseState {
public final ParseGraph order;
public final BigInteger offset;
public final Source source;
public final ImmutableList<BigInteger> iterations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that now could happen is that closeBranch(Token) is called with a Token that was not the one used for addBranch(Token). This would of course be a bug, but since these methods are public, we might need to guard for it. We could fix that by keeping track which token is used for the iteration. E.g. something like:

public final ImmutableList<Pair<Token, BigInteger>> iterations;

Whenever we call closeBranch(Token), we can verify that the Token of the current iteration is indeed the one being passed. We could even pass Token to iter() and be sure that we are iterating the expected Token (but that might be overkill).

(Note: We have a Pair class in Selection, but it is not generic.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion! I'm usually not a fan of using a Pair class, but I cannot think of an alternative, other then creating a new class, which would most likely be implemented as the Pair class. ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it in this case be an identity comparison instead of a (semantic) equality?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a good point... If we want to really make sure we are using the same object, instead of just an equal one.


public ParseState(final ParseGraph order, final Source source, final BigInteger offset) {
public ParseState(final ParseGraph order, final Source source, final BigInteger offset, final ImmutableList<BigInteger> iterations) {
this.order = checkNotNull(order, "order");
this.source = checkNotNull(source, "source");
this.offset = checkNotNegative(offset, "offset");
this.iterations = checkNotNull(iterations, "iteration");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, missing s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

}

public static ParseState createFromByteStream(final ByteStream input, final BigInteger offset) {
return new ParseState(ParseGraph.EMPTY, new ByteStreamSource(input), offset);
return new ParseState(ParseGraph.EMPTY, new ByteStreamSource(input), offset, new ImmutableList<>());
}

public static ParseState createFromByteStream(final ByteStream input) {
return createFromByteStream(input, ZERO);
}

public ParseState addBranch(final Token token) {
return new ParseState(order.addBranch(token), source, offset);
if (token.isIterable()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a oneliner... Does @jvdb approve of this heresy? 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it could, but should it? ;) The code now clearly shows the difference between the two states, which will not be as clear if it is a oneliner. @jvdb will approve this, I'm sure! :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be entirely honest, seeing multiple identical addBranch()-invocations does hurt a little... But I'm not religious about this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so you both would prefer:

return new ParseState(order.addBranch(token), source, offset, token.isIterable() ?
 iterations.add(ZERO) : iterations);

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that yes 👍

return new ParseState(order.addBranch(token), source, offset, iterations.add(ZERO));
}
return new ParseState(order.addBranch(token), source, offset, iterations);
}

public ParseState closeBranch() {
return new ParseState(order.closeBranch(), source, offset);
public ParseState closeBranch(final Token token) {
if (token.isIterable()) {
return new ParseState(order.closeBranch(), source, offset, iterations.tail);
}
return new ParseState(order.closeBranch(), source, offset, iterations);
}

public ParseState add(final ParseValue parseValue) {
return new ParseState(order.add(parseValue), source, offset);
return new ParseState(order.add(parseValue), source, offset, iterations);
}

public ParseState add(final ParseReference parseReference) {
return new ParseState(order.add(parseReference), source, offset);
return new ParseState(order.add(parseReference), source, offset, iterations);
}

public ParseState iter() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iterate/addIteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, iterate it is.

return new ParseState(order, source, offset, iterations.tail.add(iterations.head.add(ONE)));
}

public Optional<ParseState> seek(final BigInteger newOffset) {
return newOffset.compareTo(ZERO) >= 0 ? Optional.of(new ParseState(order, source, newOffset)) : Optional.empty();
return newOffset.compareTo(ZERO) >= 0 ? Optional.of(new ParseState(order, source, newOffset, iterations)) : Optional.empty();
}

public ParseState source(final ValueExpression dataExpression, final int index, final ParseState parseState, final Encoding encoding) {
return new ParseState(order, new DataExpressionSource(dataExpression, index, parseState, encoding), ZERO);
return new ParseState(order, new DataExpressionSource(dataExpression, index, parseState, encoding), ZERO, iterations);
}

public Optional<Slice> slice(final BigInteger length) {
Expand All @@ -81,20 +94,22 @@ public Optional<Slice> slice(final BigInteger length) {

@Override
public String toString() {
return getClass().getSimpleName() + "(source:" + source + ";offset:" + offset + ";order:" + order + ")";
final String iterationString = iterations.isEmpty() ? "" : ";iterations:" + iterations.toString();
return getClass().getSimpleName() + "(source:" + source + ";offset:" + offset + ";order:" + order + iterationString + ")";
}

@Override
public boolean equals(final Object obj) {
return Util.notNullAndSameClass(this, obj)
&& Objects.equals(order, ((ParseState)obj).order)
&& Objects.equals(offset, ((ParseState)obj).offset)
&& Objects.equals(source, ((ParseState)obj).source);
&& Objects.equals(source, ((ParseState)obj).source)
&& Objects.equals(iterations, ((ParseState)obj).iterations);
}

@Override
public int hashCode() {
return Objects.hash(getClass(), order, offset, source);
return Objects.hash(getClass(), order, offset, source, iterations);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright 2013-2018 Netherlands Forensic Institute
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

jvdb marked this conversation as resolved.
Show resolved Hide resolved
package io.parsingdata.metal.expression.value.reference;

import static io.parsingdata.metal.expression.value.ConstantFactory.createFromNumeric;

import java.util.Optional;

import io.parsingdata.metal.Util;
import io.parsingdata.metal.data.ImmutableList;
import io.parsingdata.metal.data.ParseState;
import io.parsingdata.metal.encoding.Encoding;
import io.parsingdata.metal.expression.value.Value;
import io.parsingdata.metal.expression.value.ValueExpression;
import io.parsingdata.metal.token.Rep;
import io.parsingdata.metal.token.RepN;
import io.parsingdata.metal.token.Token;

/**
* A {@link ValueExpression} that represents the 0-based current iteration in an
* iterable {@link Token} (e.g. when inside a {@link Rep} or {@link RepN}).
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also mention it can be checked if a Token is iterable, using a method reference link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

public class CurrentIteration implements ValueExpression {

@Override
public ImmutableList<Optional<Value>> eval(final ParseState parseState, final Encoding encoding) {
if (parseState.iterations.head == null) {
jvdb marked this conversation as resolved.
Show resolved Hide resolved
return ImmutableList.create(Optional.empty());
}
return ImmutableList.create(Optional.of(createFromNumeric(parseState.iterations.head, new Encoding())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default new Encoding() is probably good enough here. For a minute I thought we needed to pass the encoding on, but I think this is correct.

Side note: Don't we have a DEFAULT_ENCODING available somewhere? I wonder how often we do new Encoding() like this.

Copy link
Contributor Author

@mvanaken mvanaken Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is not a default encoding as constant available. We do this 6 times in core; 4 in Shorthand, 1 in CurrentOffset, and 1 in CurrentIteration. In test scope we call it 7 times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point: #245

}

@Override
public String toString() {
return getClass().getSimpleName();
}

@Override
public boolean equals(final Object obj) {
return Util.notNullAndSameClass(this, obj);
}

@Override
public int hashCode() {
return getClass().hashCode();
}

}
2 changes: 1 addition & 1 deletion core/src/main/java/io/parsingdata/metal/token/Cho.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ private Trampoline<Optional<ParseState>> iterate(final Environment environment,
}
return list.head
.parse(environment)
.map(result -> complete(() -> success(result.closeBranch())))
.map(result -> complete(() -> success(result.closeBranch(this))))
.orElseGet(() -> intermediate(() -> iterate(environment, list.tail)));
}

Expand Down
79 changes: 79 additions & 0 deletions core/src/main/java/io/parsingdata/metal/token/IterableToken.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright 2013-2018 Netherlands Forensic Institute
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.parsingdata.metal.token;
rdvdijk marked this conversation as resolved.
Show resolved Hide resolved

import io.parsingdata.metal.Trampoline;
import io.parsingdata.metal.data.Environment;
import io.parsingdata.metal.data.ParseState;
import io.parsingdata.metal.encoding.Encoding;

import java.util.Objects;
import java.util.Optional;
import java.util.function.Function;

import static io.parsingdata.metal.Trampoline.complete;
import static io.parsingdata.metal.Trampoline.intermediate;
import static io.parsingdata.metal.Util.checkNotNull;
import static io.parsingdata.metal.Util.success;

public abstract class IterableToken extends Token {
rdvdijk marked this conversation as resolved.
Show resolved Hide resolved

public final Token token;

IterableToken(String name, final Token token, Encoding encoding) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing final.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

super(name, encoding);
this.token = checkNotNull(token, "token");
}

protected final Optional<ParseState> parse(final Environment environment, final Function<Environment, Boolean> stopCondition, final Function<Environment, Optional<ParseState>> ifIterationFails) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function<Env, Boolean> = Predicate<Env> :) (unless you would want it to return a null)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tnx!

return iterate(environment.addBranch(this), stopCondition, ifIterationFails).computeResult();
}

/**
* Iteratively parse iterations of the token, given a stop condition and the logic how to handle a failed parse.
jvdb marked this conversation as resolved.
Show resolved Hide resolved
*
* @param environment the environment to apply the parse to
* @param stopCondition a function to determine when to stop the iteration
* @param ifIterationFails a function to determine how to handle a failed parse
* @return a trampolined {@code Optional<ParseState>}
*/
private Trampoline<Optional<ParseState>> iterate(final Environment environment, final Function<Environment, Boolean> stopCondition, final Function<Environment, Optional<ParseState>> ifIterationFails) {
if (stopCondition.apply(environment)) {
return complete(() -> success(environment.parseState.closeBranch(this)));
}
return token
jvdb marked this conversation as resolved.
Show resolved Hide resolved
.parse(environment)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My head is a bit foggy, but isn't this an unsafe recursive call? If you would just nest iterable tokens which do not stop at the first iteration?

.map(nextParseState -> intermediate(() -> iterate(environment.withParseState(nextParseState.iter()), stopCondition, ifIterationFails)))
.orElseGet(() -> complete(() -> ifIterationFails.apply(environment)));
}

@Override
public boolean isIterable() {
return true;
}

@Override
public boolean equals(final Object obj) {
return super.equals(obj)
&& Objects.equals(token, ((IterableToken)obj).token);
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), token);
}

}
2 changes: 1 addition & 1 deletion core/src/main/java/io/parsingdata/metal/token/Post.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public Post(final String name, final Token token, final Expression predicate, fi
protected Optional<ParseState> parseImpl(final Environment environment) {
return token
.parse(environment.addBranch(this))
.map(nextParseState -> predicate.eval(nextParseState, environment.encoding) ? success(nextParseState.closeBranch()) : failure())
.map(nextParseState -> predicate.eval(nextParseState, environment.encoding) ? success(nextParseState.closeBranch(this)) : failure())
.orElseGet(Util::failure);
}

Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/io/parsingdata/metal/token/Pre.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ protected Optional<ParseState> parseImpl(final Environment environment) {
}
return token
.parse(environment.addBranch(this))
.map(resultParseState -> success(resultParseState.closeBranch()))
.map(resultParseState -> success(resultParseState.closeBranch(this)))
.orElseGet(Util::failure);
}

Expand Down
34 changes: 4 additions & 30 deletions core/src/main/java/io/parsingdata/metal/token/Rep.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,10 @@

package io.parsingdata.metal.token;

import static io.parsingdata.metal.Trampoline.complete;
import static io.parsingdata.metal.Trampoline.intermediate;
import static io.parsingdata.metal.Util.checkNotNull;
import static io.parsingdata.metal.Util.success;

import java.util.Objects;
import java.util.Optional;

import io.parsingdata.metal.Trampoline;
import io.parsingdata.metal.data.Environment;
import io.parsingdata.metal.data.ParseState;
import io.parsingdata.metal.encoding.Encoding;
Expand All @@ -38,41 +33,20 @@
*
* @see RepN
*/
public class Rep extends Token {

public final Token token;
public class Rep extends IterableToken {

public Rep(final String name, final Token token, final Encoding encoding) {
super(name, encoding);
this.token = checkNotNull(token, "token");
super(name, token, encoding);
}

@Override
protected Optional<ParseState> parseImpl(final Environment environment) {
return iterate(environment.addBranch(this)).computeResult();
}

private Trampoline<Optional<ParseState>> iterate(final Environment environment) {
return token
.parse(environment)
.map(nextParseState -> intermediate(() -> iterate(environment.withParseState(nextParseState))))
.orElseGet(() -> complete(() -> success(environment.parseState.closeBranch())));
protected Optional<ParseState> parseImpl(Environment environment) {
return parse(environment, env -> false, env -> success(env.parseState.closeBranch(this)));
}

@Override
public String toString() {
return getClass().getSimpleName() + "(" + makeNameFragment() + token + ")";
}

@Override
public boolean equals(final Object obj) {
return super.equals(obj)
&& Objects.equals(token, ((Rep)obj).token);
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), token);
}
rdvdijk marked this conversation as resolved.
Show resolved Hide resolved

}
Loading