Make ParserD consistent with ParserK#2811
Conversation
adithyaov
force-pushed
the
parser-mod
branch
2 times, most recently
from
2c79b5b to
039bc03
Compare
adithyaov
force-pushed
the
parser-mod
branch
7 times, most recently
from
d047c6a to
0dc282e
Compare
adithyaov
force-pushed
the
parser-mod
branch
from
0dc282e to
3e7bda1
Compare
harendra-kumar
force-pushed
the
parser-mod
branch
from
8ef69ca to
5ff777a
Compare
| assert (n >= 0) | ||
| (return $ Continue (- n) (parseCont (count - n) pst1)) | ||
| ParserD.SDone m b -> | ||
| let n = 1 - m |
There was a problem hiding this comment.
This looks incorrect based on the IMPORTANT note above. But why is no test failing?
There was a problem hiding this comment.
Was the IMPORTANT note incorrect?
| assert (n >= 0) | ||
| (return $ Continue (- n) (parseCont (count - n) pst1)) | ||
| ParserD.SDone m b -> | ||
| let n = 1 - m |
There was a problem hiding this comment.
This looks incorrect based on the IMPORTANT note above. But why is no test failing?
harendra-kumar
force-pushed
the
parser-mod
branch
from
5ff777a to
1cdb2d3
Compare
| let n = Prelude.length backBuf | ||
| arr0 = fromListN n (Prelude.reverse backBuf) | ||
| return (Left (ParseError err), StreamK.fromPure arr0) | ||
| -} |
There was a problem hiding this comment.
Why did you comment this out?
There was a problem hiding this comment.
This is redundant and requires unnecessary maintenance, should perhaps be removed.
| if not (cond a) | ||
| then return res | ||
| else extractStep pextract res | ||
| else |
There was a problem hiding this comment.
Was the previous code on origin/master incorrect?
There was a problem hiding this comment.
I don't think so. It has to be changed because of the extract changes.
| -- If the parser is backtracking we let it backtrack even if the | ||
| -- predicate is true. | ||
| fmap (first Left') $ case res of | ||
| SPartial 1 s1 -> mapCount (+1) <$> pextract s1 |
There was a problem hiding this comment.
Please verify: The reason we do this is because extract does not need to account for the current element whereas step has to.
| let taken = cnt1 - n | ||
| SPartial _ _ -> error "takeP: SPartial in extract" | ||
| SPartial n s -> do | ||
| let taken = cnt + n |
There was a problem hiding this comment.
Shouldn't this be cnt + n - 1?
There was a problem hiding this comment.
Oh nvm, I misread. cnt1 vs cnt. cnt1 == cnt + 1
| pRes <- pstep pst x | ||
| case pRes of | ||
| PR.Partial 0 pst1 -> | ||
| PR.SPartial 1 pst1 -> |
There was a problem hiding this comment.
The accounting in goExtract should be similar to goStop?
Partial n == SPartial -n
So SPartial 0 here?
There was a problem hiding this comment.
Similar changes need to be made to the rest of the function.
There was a problem hiding this comment.
We are running pstep pst x above feeding it one element, therefore we need to take into account the element we fed. In goStop we are calling extract instead which is different.
| -- IMPORTANT: the n here is from the byte stream parser, that means | ||
| -- it is the backtrack element count and not the stream position | ||
| -- index into the current input chunk. |
There was a problem hiding this comment.
Is this note incorrect?
There was a problem hiding this comment.
yes, this is what we changed to make ParserD and ParserK consistent.
adithyaov
left a comment
adithyaov
left a comment
There was a problem hiding this comment.
1 bug. The rest looks good.
- Stream.parse - Producer.parse - Stream.parseMany - StreamK.parseD - Array.parseBreakChunksK
harendra-kumar
force-pushed
the
parser-mod
branch
from
40afdc0 to
7d2e88f
Compare
harendra-kumar
force-pushed
the
parser-mod
branch
from
7d2e88f to
0ddbd48
Compare
2 participants
This needs to be reviewed very carefully.