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.

Sign up for GitHub

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

Change how 'Disable Formatting' works #4496

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Change how 'Disable Formatting' works #4496

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73636ee
Always indent, even when PrintFormatting is disabled
ChrisJefferson May 18, 2021
3796ef2
Fix SetPrintFormattingStatus for *stdout*
ChrisJefferson May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 11 additions & 16 deletions src/io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,20 +1202,12 @@ static void PutChrTo(TypOutputFile * stream, Char ch)

/* '\01', increment indentation level */
if ( ch == '\01' ) {

if (!stream->format)
return;

/* add hint to break line */
addLineBreakHint(stream, stream->pos, 16*stream->indent, 1);
}

/* '\02', decrement indentation level */
else if ( ch == '\02' ) {

if (!stream->format)
return;

/* if this is a better place to split the line remember it */
addLineBreakHint(stream, stream->pos, 16*stream->indent, -1);
}
Expand Down Expand Up @@ -1249,12 +1241,11 @@ static void PutChrTo(TypOutputFile * stream, Char ch)

/* and dump it from the buffer */
stream->pos = 0;
if (stream->format)
{
/* indent for next line */
for ( i = 0; i < stream->indent; i++ )
stream->line[ stream->pos++ ] = ' ';
}

/* indent for next line */
for ( i = 0; i < stream->indent; i++ )
stream->line[ stream->pos++ ] = ' ';

/* reset line break hints */
stream->hints[0] = -1;

Expand Down Expand Up @@ -1866,9 +1857,13 @@ static Obj FuncSET_PRINT_FORMATTING_STDOUT(Obj self, Obj val)
TypOutputFile * output = IO()->Output;
if (!output)
ErrorMayQuit("SET_PRINT_FORMATTING_STDOUT called while no output is opened\n", 0, 0);
while (output->prev)
Copy link
Member

Choose a reason for hiding this comment

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

This change is wrong, it should affect stdout, not the top open file, which may be different from stdout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this isn't currently right.

There is currently a problem, whereby we end up with two copies of stdout on the stack. By chucking in some prints I see the stack of Outputs is two "stdout". The old code would change the bottom one, but then the top (currently active) one would not have printing disabled.

I could explicitly search down the stack for the first "stdout" (which would mean file=1), and change the formatting of that, or investigate why we get multiple stdout on the output stack.

Copy link
Member

Choose a reason for hiding this comment

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

We get multiple stdout (and stderr) on the stack because that's the whole point of the stack: we start with stdout open and run code; that code opens a file and runs some more code; which opens stdout again. Now we have a stack with three levels: stdout, that file, again stdout. If the currently running code closes its output, the active output should again point at the file -- the bottommost "stdout" should still stay open, though.

The problematic part thus is not that stdout (and stderr) can be multiple times on the stack; rather it is that they do not share data: neither the formatting hints nor the line cache. This already causes issues with printing to stderr: if your terminal is 80 chars wide, and you print 50 chars to stdout, then open stderr and print another 45 chars, GAP fails to realize that a wraparound has occurred, because the stderr output stream is not aware of the 50 chars printed before it. And when stderr is closed, then stdout still thinks that 50 chars were printed to the terminal, when really it was 95. See also issue #1197.

There are several ways to address this. One crude but easy to implement approach would be to detect whenever we open/close stdout or stderr, and add some special treatment: when opening a new one, make sure to copy the content of the TypOutputFile struct of the any previously opened stdout/stderr over (well, at least part of it; maybe pos/hint/indent/...?). Conversely, when closing, copy it back to the previously opened stdout/stderr. Of course variations are possible, e.g. there could be a static TypOutputFile Stdout in io.c, and we simply sync with that resp. use that whenever output goes to stdout or stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that might well be a better fix. I've tweaked this PR so it now walks the stack of open Outputs and changes the formatting on each one which is *stdout*. That still doesn't fix the other problems you discuss of course.

while (output->prev) {
// *stdout* is always file '1'
if (output->file == 1) {
output->format = (val != False);
}
output = output->prev;
output->format = (val != False);
}
return val;
}

Expand Down