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 15 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright 2013-2016 Netherlands Forensic Institute
Copy link
Collaborator

Choose a reason for hiding this comment

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

2018?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

*
* 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 java.math.BigInteger.ONE;
import static java.math.BigInteger.ZERO;

import static io.parsingdata.metal.Trampoline.complete;
import static io.parsingdata.metal.Trampoline.intermediate;
import static io.parsingdata.metal.expression.value.ConstantFactory.createFromNumeric;

import java.math.BigInteger;
import java.util.Optional;

import io.parsingdata.metal.Trampoline;
import io.parsingdata.metal.Util;
import io.parsingdata.metal.data.ImmutableList;
import io.parsingdata.metal.data.ParseGraph;
import io.parsingdata.metal.data.ParseItem;
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;

/**
* A {@link ValueExpression} that represents the current iteration in an
* iterable {@link io.parsingdata.metal.token.Token} (e.g. when inside a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps import the class names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* {@link io.parsingdata.metal.token.Rep} or
* {@link io.parsingdata.metal.token.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) {
final ParseGraph currentIterable = findCurrentIterable(parseState.order, ParseGraph.EMPTY).computeResult();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool way to calculate the iteration by the way 😄

if (currentIterable.isEmpty()) { return ImmutableList.create(Optional.empty()); }

final BigInteger currentIteration = countIterable(currentIterable, ZERO).computeResult();
return ImmutableList.create(Optional.of(createFromNumeric(currentIteration, new Encoding())));
}

private Trampoline<ParseGraph> findCurrentIterable(final ParseItem item, final ParseGraph iterableCandidate) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would really like some inline comments or javadoc stating what these things do 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not applicable anymore. ;) These methods are removed, because of the new implementation.

if (!item.isGraph()) { return complete(() -> iterableCandidate); }
Copy link
Collaborator

@ccreeten ccreeten Aug 1, 2018

Choose a reason for hiding this comment

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

What would happen if I would do something like rep(def("val", CURRENT_ITERATION)) or rep(tie(any("val"), CURRENT_ITERATION))? Because when I eval the expression, there is no value to stop at? Seems like it would blow up?

Copy link
Contributor Author

@mvanaken mvanaken Aug 2, 2018

Choose a reason for hiding this comment

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

If you mean infinite loop, no, but there will be a null pointer exception, because the head becomes null. Nice catch! Will add item == null to this if statement and add tests.

Copy link
Contributor Author

@mvanaken mvanaken Aug 2, 2018

Choose a reason for hiding this comment

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

Ah, I see your point now. Because a def of size 0 is not added to the parseGraph. However, then the iterable is in fact empty, which will return an empty value and the parsing will fail, see line 50. So it will not blow up.

It is a nice discussion though what you would expect to be "an iteration". Should a parsing attempt, where eventually 0 bytes are parsed, be considered an iteration? Also, would we expect rep(def("val", add(CURRENT_ITERATION, con(1)))) to work?

Copy link
Collaborator

@ccreeten ccreeten Aug 2, 2018

Choose a reason for hiding this comment

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

Actually, if I read the code as is, I think your first comment does happen (I indeed meant something like a NPE)? Because there is no value yet when you eval CURRENT_ITERATION, and the only way the recursion stops is if it encounters a value? Could you add these samples as a test?

As to the discussion, I would probably expect your example to work, because we are in fact in an iteration? I also think conceptually a 0 bytes parsed should still be considered an iteration, as I feel it is the iteration that happens, not what happens inside the iteration (wow, confusing sentence 😄). What would be the downsides of this, or what problems would it pose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented CURRENT_INDEX differently. Instead of extracting it from ParseGraph, it is kept in the ParseState. I added tests for examples similar to rep(def("val", add(CURRENT_ITERATION, con(1)))).

I will need to improve the code a little before I commit.

if (item.getDefinition().isIterable()) {
return intermediate(() -> findCurrentIterable(item.asGraph().head, item.asGraph()));
}
return intermediate(() -> findCurrentIterable(item.asGraph().head, iterableCandidate));
}

private Trampoline<BigInteger> countIterable(final ParseGraph graph, final BigInteger count) {
if (!graph.isEmpty()) { return intermediate(() -> countIterable(graph.tail, count.add(ONE))); }
return complete(() -> count.subtract(ONE));
}

@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();
}

}
5 changes: 5 additions & 0 deletions core/src/main/java/io/parsingdata/metal/token/Rep.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ private Trampoline<Optional<ParseState>> iterate(final Environment environment)
.orElseGet(() -> complete(() -> success(environment.parseState.closeBranch())));
}

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

@Override
public String toString() {
return getClass().getSimpleName() + "(" + makeNameFragment() + token + ")";
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/io/parsingdata/metal/token/RepN.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ private Trampoline<Optional<ParseState>> iterate(final Environment environment,
.orElseGet(() -> complete(Util::failure));
}

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

@Override
public String toString() {
return getClass().getSimpleName() + "(" + makeNameFragment() + token + "," + n + ")";
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/io/parsingdata/metal/token/Token.java
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ public boolean isLocal() {
return true;
}

public boolean isIterable() {
return false;
}

public Token getCanonical(final ParseState parseState) {
return this;
}
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/io/parsingdata/metal/token/While.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ private Trampoline<Optional<ParseState>> iterate(final Environment environment)
return complete(() -> success(environment.parseState.closeBranch()));
}

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

@Override
public String toString() {
return getClass().getSimpleName() + "(" + makeNameFragment() + token + "," + predicate + ")";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
import io.parsingdata.metal.expression.value.bitwise.ShiftLeft;
import io.parsingdata.metal.expression.value.bitwise.ShiftRight;
import io.parsingdata.metal.expression.value.reference.Count;
import io.parsingdata.metal.expression.value.reference.CurrentIteration;
import io.parsingdata.metal.expression.value.reference.CurrentOffset;
import io.parsingdata.metal.expression.value.reference.First;
import io.parsingdata.metal.expression.value.reference.Last;
Expand Down Expand Up @@ -191,7 +192,7 @@ public static Collection<Object[]> data() throws IllegalAccessException, Invocat
And.class, Or.class, ShiftLeft.class, ShiftRight.class, Add.class, Div.class, Mod.class, Mul.class,
io.parsingdata.metal.expression.value.arithmetic.Sub.class, Cat.class, Nth.class, Elvis.class,
FoldLeft.class, FoldRight.class, Const.class, Expand.class, Bytes.class, CurrentOffset.class,
FoldCat.class,
FoldCat.class, CurrentIteration.class,
// Expressions
Eq.class, EqNum.class, EqStr.class, GtEqNum.class, GtNum.class, LtEqNum.class, LtNum.class,
io.parsingdata.metal.expression.logical.And.class, io.parsingdata.metal.expression.logical.Or.class,
Expand Down
5 changes: 4 additions & 1 deletion core/src/test/java/io/parsingdata/metal/ToStringTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import static io.parsingdata.metal.Shorthand.CURRENT_ITERATION;
import static io.parsingdata.metal.Shorthand.CURRENT_OFFSET;
import static io.parsingdata.metal.Shorthand.SELF;
import static io.parsingdata.metal.Shorthand.add;
Expand Down Expand Up @@ -121,7 +122,7 @@ private Token t() {
}

private ValueExpression v() {
return fold(foldLeft(foldRight(rev(bytes(neg(add(div(mod(mul(sub(cat(last(ref(n()))), first(nth(exp(ref(n()), con(1)), con(1)))), con(1)), cat(ref(n()), ref(t()))), add(SELF, add(offset(ref(n())), add(CURRENT_OFFSET, count(ref(n())))))), elvis(ref(n()), ref(n())))))), Shorthand::add, ref(n())), Shorthand::add), Shorthand::add, ref(n()));
return fold(foldLeft(foldRight(rev(bytes(neg(add(div(mod(mul(sub(cat(last(ref(n()))), first(nth(exp(ref(n()), con(1)), con(1)))), sub(CURRENT_ITERATION, con(1))), cat(ref(n()), ref(t()))), add(SELF, add(offset(ref(n())), add(CURRENT_OFFSET, count(ref(n())))))), elvis(ref(n()), ref(n())))))), Shorthand::add, ref(n())), Shorthand::add), Shorthand::add, ref(n()));
}

@Test
Expand All @@ -148,6 +149,8 @@ public void specialExpressions() {
assertEquals("Self", SELF.toString());
assertTrue(v().toString().contains("CurrentOffset"));
assertEquals("CurrentOffset", CURRENT_OFFSET.toString());
assertTrue(v().toString().contains("CurrentIteration"));
assertEquals("CurrentIteration", CURRENT_ITERATION.toString());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright 2013-2016 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.expression.value.reference;

import static io.parsingdata.metal.Shorthand.CURRENT_ITERATION;
import static io.parsingdata.metal.Shorthand.con;
import static io.parsingdata.metal.Shorthand.def;
import static io.parsingdata.metal.Shorthand.eq;
import static io.parsingdata.metal.Shorthand.not;
import static io.parsingdata.metal.Shorthand.rep;
import static io.parsingdata.metal.Shorthand.repn;
import static io.parsingdata.metal.Shorthand.seq;
import static io.parsingdata.metal.Shorthand.whl;
import static io.parsingdata.metal.util.EncodingFactory.enc;
import static io.parsingdata.metal.util.ParseStateFactory.stream;

import java.util.Arrays;
import java.util.Collection;

import org.junit.runners.Parameterized;

import io.parsingdata.metal.token.Token;
import io.parsingdata.metal.util.ParameterizedParse;

public class CurrentIterationTest extends ParameterizedParse {

public static final Token VALUE_EQ_ITERATION = def("value", con(1), eq(CURRENT_ITERATION));
public static final Token VALUE_NOT_EQ_ITERATION = def("value", con(1), not(eq(CURRENT_ITERATION)));
public static final Token VALUE_EQ_255 = def("value", con(1), eq(con(255)));

@Parameterized.Parameters(name="{0} ({4})")
public static Collection<Object[]> data() {
return Arrays.asList(new Object[][] {
{ "[0, 1, 2, 3, 255] rep(CURRENT_ITERATION), def(255)", seq(rep(VALUE_EQ_ITERATION), VALUE_EQ_255), stream(0, 1, 2, 3, 255), enc(), true },
{ "[0, 1, 2, 3] repn=4(CURRENT_ITERATION)", repn(VALUE_EQ_ITERATION, con(4)), stream(0, 1, 2, 3), enc(), true },
{ "[255, 0, 1, 2, 3, 255] def(255), while<3(CURRENT_ITERATION), def (255)", seq(VALUE_EQ_255, whl(VALUE_EQ_ITERATION, not(eq(con(3)))), VALUE_EQ_255), stream(255, 0, 1, 2, 3, 255), enc(), true },
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: the description of the test should actually be while!=3 instead of while<3 right?

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, correct. I will adjust it.

{ "[0, 0, 1, 2, 1, 0, 1, 2] repn=2(CURRENT_ITERATION, repn=3(CURRENT_ITERATION))", repn(seq(VALUE_EQ_ITERATION, repn(VALUE_EQ_ITERATION, con(3))), con(2)), stream(0, 0, 1, 2, 1, 0, 1, 2), enc(), true },
{ "[0, 1] seq(CURRENT_ITERATION, ...)", seq(VALUE_EQ_ITERATION, VALUE_EQ_ITERATION), stream(0, 1), enc(), false },
{ "[0] CURRENT_ITERATION", VALUE_EQ_ITERATION, stream(0), enc(), false },
{ "[0, 1] seq(!CURRENT_ITERATION, ...)", seq(VALUE_NOT_EQ_ITERATION, VALUE_NOT_EQ_ITERATION), stream(0, 1), enc(), true },
{ "[0] !CURRENT_ITERATION", VALUE_NOT_EQ_ITERATION, stream(0), enc(), true }
});
}

}