-
Notifications
You must be signed in to change notification settings - Fork 51
Combinator variations #177
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
Comments
Here is what the benchmarks from #176 (comment) show.
|
Also, why are some of the combinators still blowing the stack? |
Ideally the non Rec versions should still be stack safe. I don’t think we should replace them without understanding why they blow the stack. |
Ok. |
I can't replicate the stack issues with module Main where
import Prelude
import Data.Array as Array
import Data.List (List)
import Data.String (joinWith)
import Effect (Effect)
import Effect.Class.Console (logShow)
import Parsing (Parser, runParser)
import Parsing.Combinators (sepBy)
import Parsing.String (string)
testString :: String
testString = joinWith " " $ Array.replicate 100000 ("A")
testParser :: Parser String (List String)
testParser = sepBy (string "A") (string " ")
main :: Effect Unit
main = do
let res = runParser testString testParser
logShow res I get the full result printed. My guess would be that there is a stack issue elsewhere, not necessarily in the actual execution of the parser. |
Oh, interesting! I think it's because the parser actually fails to parse. You have "23" repeated, and are using sepBy, which will result in a parse error. Your original example is blowing the stack in the |
@natefaubion fixed the stack safety of |
Or maybe we should try to do #155 first before we make these decisions. |
I fixed the parse errors that you noticed, here are the new benchmarks: #180 (comment)
Or maybe we should try to do #155 first before we make these decisions. |
I think what I'm seeing here #181 (comment)
|
I still don't understand why |
|
We need to figure out which combinators to keep and which are redundant after the CPS refactor #154
The text was updated successfully, but these errors were encountered: