Skip to content
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

Allow setting ContinuationLines to 0, meaning infinity (fixes #392) #394

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Conversation

magv
Copy link
Contributor

@magv magv commented Sep 2, 2021

Allow disabling the continuation line limit by setting ContinuationLines to 0.

Also fix the documentation: the continuation line limit applies not only for Fortran, but for all formats.

This fixes issue #392.

Also fix the documentation: the continuation line limit applies
not only for Fortran, but for all formats.
@magv
Copy link
Contributor Author

magv commented Sep 13, 2021

Anyone? I'd imagine this change to be uncontroversial.

@benruijl
Copy link
Collaborator

I am also for this change. In another thread @vermaseren mentioned that there may be some fixed-size buffers. Are we sure that this change does not produce buffer overflows?

@magv
Copy link
Contributor Author

magv commented Sep 13, 2021

There's a fixed-size buffer that holds the content of a line; this one is flushed every line. The line continuation marks are just printed every few lines or so, no buffer involved. I think this change is safe.

@vermaseren
Copy link
Owner

vermaseren commented Sep 13, 2021 via email

@magv
Copy link
Contributor Author

magv commented Sep 13, 2021

There's unfortunately also some logic to insert spaces, &, and \ marks into the line buffers that would need to be modified to fully allow printing without newlines -- and that logic is a bit too tangled for me, so at the moment I've given up on changing that. Instead of the full solution, at least allowing to suppress the line continuation break marks (the _ += sequences) would make handling FORM output a bit more bearable for us -- hence this pull request.

Copy link
Collaborator

@tueda tueda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is OK (unless it interferes with the version 5.0 development source code).
Maybe you can add a test (like check/features.frm#Issue392) in another commit, which guarantees this feature won't be broken.

@magv
Copy link
Contributor Author

magv commented Sep 13, 2021

Here's a test case.

Copy link
Collaborator

@tueda tueda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@benruijl
Copy link
Collaborator

Is this ready for merging?

@magv
Copy link
Contributor Author

magv commented Dec 21, 2021

If you're asking me, then yes. Not sure about others' opinions.

@benruijl benruijl merged commit 2a37fd2 into vermaseren:master Dec 21, 2021
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.

4 participants