There are options in the review system for viewing the latest commit, or all commits. That may work well with the workflow you suggest here.
We definitely want the squash done before the final rebase and merge. ________________________________ From: Steve Lawrence <[email protected]> Sent: Tuesday, November 7, 2017 12:44:26 PM To: [email protected]; mbeckerle; [email protected] Subject: Re: [GitHub] incubator-daffodil pull request #4: Ensure marks are always returned to the ... Interesting, I added code comments based on the review, squashed the changes, and did a force push. GitHub automatically updated the pull request with the new branch, but lost Mike's comments on the pull request. I'm not sure this is too big of deal since comments are logged in both JIRA and the mailing list, but it's something to be aware of. It could make additional comments/changes to the pull request hard to follow, though. Perhaps the workflow should be to push all updates as new commits (i.e. no --force). Once enough +1's are received, the changes should be squashed, force pushed, and then the pull request can be accepted via GitHub's "Rebase and Merge" option? On 11/07/2017 10:03 AM, mbeckerle wrote: > Github user mbeckerle commented on a diff in the pull request: > > https://github.com/apache/incubator-daffodil/pull/4#discussion_r149394482 > > --- Diff: > daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/parsers/RepParsers.scala > --- > @@ -207,87 +253,129 @@ class RepUnboundedParser(occursCountKind: > OccursCountKind.Value, rParser: Parser > extends RepParser(-1, rParser, erd, "Unbounded") { > > def parseAllRepeats(initialState: PState): Unit = { > + var startState: PState.Mark = null > + var priorState: PState.Mark = null > + var markLeakCausedByException = false > + > Assert.invariant(initialState.processorStatus eq Success) > - val startState = initialState.mark("RepUnboundedParser1") > - val pstate = initialState > - var priorState = initialState.mark("RepUnboundedParser2") > - var returnFlag = false > - while (!returnFlag && (pstate.processorStatus eq Success)) { > - > - // erd.maxOccurs.foreach { maxOccurs => > - // if ((occursCountKind == OccursCountKind.Implicit) && > - // (maxOccurs == -1)) { > - // erd.minOccurs.foreach { minOccurs => > - // if (pstate.mpstate.arrayPos - 1 <= minOccurs) { > - // // Is required element > - // // Need to trigger default value creation > - // // in right situations (like the element is > defaultable) > - // // This is relatively easy for simple types > - // // for complex types, defaulting is trickier as one > - // // must recursively traverse the type, and then > determine one > - // // has not advanced the data at all. > - // } > - // } > - // } > - // } > - > - // > - // Every parse is a new point of uncertainty. > - pstate.pushDiscriminator > - if (pstate.dataProc.isDefined) > pstate.dataProc.get.beforeRepetition(pstate, this) > - > - try { > - rParser.parse1(pstate) > - } catch { > - case sde: SchemaDefinitionDiagnosticBase => { > - pstate.discard(startState) > - throw sde > - } > - } > > - if (pstate.dataProc.isDefined) > pstate.dataProc.get.afterRepetition(pstate, this) > - if (pstate.processorStatus ne Success) { > - // > - // Did not succeed > - // > - // Was a discriminator set? > + try { > + val pstate = initialState > + startState = initialState.mark("RepUnboundedParser1") > + priorState = initialState.mark("RepUnboundedParser2") > + var returnFlag = false > + while (!returnFlag && (pstate.processorStatus eq Success)) { > + > + // erd.maxOccurs.foreach { maxOccurs => > + // if ((occursCountKind == OccursCountKind.Implicit) && > + // (maxOccurs == -1)) { > + // erd.minOccurs.foreach { minOccurs => > + // if (pstate.mpstate.arrayPos - 1 <= minOccurs) { > + // // Is required element > + // // Need to trigger default value creation > + // // in right situations (like the element is > defaultable) > + // // This is relatively easy for simple types > + // // for complex types, defaulting is trickier as > one > + // // must recursively traverse the type, and then > determine one > + // // has not advanced the data at all. > + // } > + // } > + // } > + // } > + > // > - if (pstate.discriminator == true) { > - // we fail the whole RepUnbounded, because there was a > discriminator set > - // before the failure. > - pstate.reset(startState) > - // no need discard priorState, that is implicitly discarded by > resetting the startState > - } else { > + // Every parse is a new point of uncertainty. > + pstate.pushDiscriminator > + if (pstate.dataProc.isDefined) > pstate.dataProc.get.beforeRepetition(pstate, this) > + > + try { > + rParser.parse1(pstate) > + } catch { > + case sde: SchemaDefinitionDiagnosticBase => { > + pstate.discard(startState) > + startState = null > + priorState = null > + throw sde > + } > + } > + > + if (pstate.dataProc.isDefined) > pstate.dataProc.get.afterRepetition(pstate, this) > + if (pstate.processorStatus ne Success) { > // > - // no discriminator, so suppress the failure. Loop terminated > with prior element. > + // Did not succeed > + // > + // Was a discriminator set? > // > + if (pstate.discriminator == true) { > + // we fail the whole RepUnbounded, because there was a > discriminator set > + // before the failure. > + pstate.reset(startState) > + startState = null > + priorState = null > + // no need discard priorState, that is implicitly discarded > by resetting the startState > + } else { > + // > + // no discriminator, so suppress the failure. Loop > terminated with prior element. > + // > > - log(LogLevel.Debug, "Failure suppressed. This is normal > termination of a occursCountKind='parsed' array.") > - pstate.reset(priorState) > - pstate.discard(startState) > - } > - returnFlag = true > - } else { > - // Success > - // Need to check for forward progress > - if (pstate.bitPos =#= priorState.bitPos0b) { > - pstate.discard(priorState) // didn't move, but might have > assigned variables, have to undo those. > - pstate.discard(startState) > - PE(pstate, > - "RepUnbounded - No forward progress at byte %s. Attempt to > parse %s " + > - "succeeded but consumed no data.\nPlease re-examine your > schema to correct this infinite loop.", > - pstate.bytePos, erd.diagnosticDebugName) > + log(LogLevel.Debug, "Failure suppressed. This is normal > termination of a occursCountKind='parsed' array.") > + pstate.reset(priorState) > + pstate.discard(startState) > + startState = null > + priorState = null > + } > returnFlag = true > } else { > - pstate.discard(priorState) > - priorState = pstate.mark("RepUnboundedParser3") > - pstate.mpstate.moveOverOneArrayIndexOnly > - returnFlag = false > + // Success > + // Need to check for forward progress > + if (pstate.bitPos =#= priorState.bitPos0b) { > + pstate.discard(priorState) // didn't move, but might have > assigned variables, have to undo those. > + pstate.discard(startState) > + startState = null > + priorState = null > + PE(pstate, > + "RepUnbounded - No forward progress at byte %s. Attempt to > parse %s " + > + "succeeded but consumed no data.\nPlease re-examine your > schema to correct this infinite loop.", > + pstate.bytePos, erd.diagnosticDebugName) > + returnFlag = true > + } else { > + pstate.discard(priorState) > + priorState = pstate.mark("RepUnboundedParser3") > + pstate.mpstate.moveOverOneArrayIndexOnly > + returnFlag = false > + } > + } > + pstate.popDiscriminator > + } > + Assert.invariant(returnFlag == true) > + } catch { > + case t: Throwable => { > + if (priorState != null || startState != null) { > + markLeakCausedByException = true > + if (!t.isInstanceOf[SchemaDefinitionDiagnosticBase] && > !t.isInstanceOf[UnsuppressableException]) { > --- End diff -- > > I was going to comment on shared logic between this and the > AltCompParser, but really I think sharing would be more complex than > duplicating in this case. I suggest just a comment in the AltCompParser > directing developers to the corresponding part of Rep Parsers, and vice > versa, so that nobody edits one without also considering the other. > > I had to think about whether there are other places that need to deal > with marks before I convinced myself there are only these two. It would be > good to write that down in a comment in here. > > > --- >
