Skip to content

Fix ICE when comment on last line on global object, closes #344 #354

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

Closed

Conversation

filipsajdak
Copy link
Contributor

In the current implementation of cppfront (f83ca9), the following code

o: k = (0, 1); // Initializes `y` from `1`, then `m` from `y`.

Cause ICE not all comments were printed error.

This change introduces a check if all comments were printed after phase1. If there are such comments, they are printed.

All regression tests pass. Closes #344

In the current implementation of cppfront (f83ca9) the following code
```cpp
o: k = (0, 1); // Initializes `y` from `1`, then `m` from `y`.
```
Cause ICE `not all comments were printed` error.

This change introduce a check if all comments were printed after phase1.
If there are such comments there are printed.

All regression tests pass. Closes hsutter#344
@JohelEGP
Copy link
Contributor

#351 (comment) works now. Thank you.

@hsutter hsutter closed this in d7adb8f Apr 15, 2023
@filipsajdak filipsajdak deleted the fsajdak-fix-comment-on-last-line branch April 16, 2023 00:02
zaucy pushed a commit to zaucy/cppfront that referenced this pull request Dec 5, 2023
…loses hsutter#351, closes hsutter#354

Thanks for these test cases. The common issue was that occasionally an emitted piece of Cpp1 contained a newline character, which was not accounted for in the code that did comment merging.

Thanks also for the PRs for proposed fixes. I looked at those too and tried them out. In the end, I found that what worked best and most generally was to break apart strings that contain newlines into one chunk per line, and this allowed the comment merging to happen correctly by avoiding the hidden-to-the-comment-merger line change.

So this commit is going with that solution rather than the other proposed PRs, but the other PRs helped find this root cause and shed light on a general solution, as did the test cases. Thanks again for both!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ICE: not all comments were printed
2 participants