-
Notifications
You must be signed in to change notification settings - Fork 20
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
More efficient breakString
#177
base: develop
Are you sure you want to change the base?
Conversation
breakString
(#169)breakString
cac00a5
to
195de1b
Compare
195de1b
to
f99b977
Compare
This was a bit of a pain because I did not consider that building the result from right to left means that the accumulator cannot be used to pass in a prefix, I'll squash merge then. I am confident this is correct now, it both passes all printing tests in this repo, as well as all cram tests in the goblint repo which also exercise this function. |
This now traverses the string right-to-left (because the It looks like |
Judging by the comments a few lines below, this right-heavy tree actually seems preferable over the one they had previously: Lines 116 to 119 in f5ee39b
|
Lines 227 to 231 in f5ee39b
It seems like the new function I give constructs more or less the tree you would get after calling |
That's true, which maybe is a good thing. So it's not clear to me what of the old behavior should be retained (for the sake of just optimizing) and what's actually inconsequential (given how |
Given we did not encounter a stack overflow for goblint/analyzer#1513 where I also used an non tail-recursive helper, I would assume that this is also not problematic here. If we run into any trouble, we can always go back to a more faithful replacement. |
Closes #169.
See also goblint/analyzer#1513 that also has some runtime comparisons.