Skip to content

Conversation

@Kojoley
Copy link
Collaborator

@Kojoley Kojoley commented Feb 18, 2022

  • Do not rely on iterator rollback on fail: Preparation to relax iterator rollback policy, which is also currently violated by not rolling back skipper almost everywhere, so this should fix postskip disabling.
  • Remove unneeded iterator rollback: Under the current iterator rollback policy rollbacks could be omitted in compound parser which don't advance iterator themselves because the called subparser is responsible for rollback on failure. In reality these rollbacks are undoing skipper work what renders as inconsistency in error handling position (p vs p >> eps or repeat(1)[p]).

Fixes #712

Preparation to relax iterator rollback policy, which is also currently violated by not rolling back skipper almost everywhere, so this should fix postskip disabling.
Under the current iterator rollback policy rollbacks could be omitted in compound parser which don't advance iterator themselves because the called subparser is responsible for rollback on failure. In reality these rollbacks are undoing skipper work what renders as inconsistency in error handling position (`p` vs `p >> eps` or `repeat(1)[p]`).
@djowel
Copy link
Collaborator

djowel commented Feb 18, 2022

This is a major move! Nice work @Kojoley.

It would be impossible to determine the correctness by just looking at the code. So I suggest adding tests for each of the changes (if there are no such tests yet).

@Kojoley
Copy link
Collaborator Author

Kojoley commented Mar 1, 2022

I was planning to add more tests to have better coverage, but now I do not know when I will be able to continue the PR.
As you may know, Russia, my home country, has invaded Ukraine and currently is in an active war. I am very sad and depressed, millions of peaceful Ukrainians are suffering from inhumane decisions made by politicians which do not represent me and I had never chosen, but I still feel responsible and guilty. The only thing I could do now is to publicly say that I do not support the war with Ukraine and it should be stopped immediately.

@djowel
Copy link
Collaborator

djowel commented Mar 1, 2022

I was planning to add more tests to have better coverage, but now I do not know when I will be able to continue the PR. As you may know, Russia, my home country, has invaded Ukraine and currently is in an active war. I am very sad and depressed, millions of peaceful Ukrainians are suffering from inhumane decisions made by politicians which do not represent me and I had never chosen, but I still feel responsible and guilty. The only thing I could do now is to publicly say that I do not support the war with Ukraine and it should be stopped immediately.

I have high regard and respect for you, @Kojoley. This message further affirms that. I applaud you for making a statement. Peace!

@cppljevans
Copy link
Contributor

cppljevans commented Mar 1, 2022 via email

@saki7
Copy link
Collaborator

saki7 commented May 9, 2025

This is a great fix. We really need to get this merged. All we need to do is resolving the conflicts and adding some unit tests. At first glance, the implementation seems ok.

@saki7
Copy link
Collaborator

saki7 commented Sep 25, 2025

#833 (comment)

It's been quite a while and I don't remember much, it's most likely the case #719 was supposed to fix. I remember I almost finished the patch but haven't updated the PR. I can try to find and upload my latest version of that PR, but I'm not going to rebase and resolve merge conflicts that emerged from the code churn that has happened since then, sorry.

@Kojoley Could you push the recent version? If you don't have time then feel free to push it as-is. I'm not entirely sure if it can be merged into feature-frozen X3, but your work is still important and I might be able to adopt it to the X4 codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

X3 3.0.10 error_handler where() is wrong

4 participants