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

Fix a long standing bug which prevented SetPrintFormattingStatus( "*stdout*", false ); from working as expected #4132

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Oct 10, 2020

Instead of pre-allocating a fixed number of TypOutputFile instances on each thread, we allocate the required storage dynamically on the stack. This arguably makes it easier to reason about the global state of GAP.

It also enables future simplifications and improvements, e.g. to fix issues were switching between stdout and errout breaks tracking of the output state, which leads to GAP expecting resp. inserting linebreaks in the wrong spots.

This includes PR #4122 which should be reviewed and merged first. DONE

Fixes #1308

Text for release notes:

Fix a long standing bug which prevented SetPrintFormattingStatus( "*stdout*", false ); from working as expected

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 10, 2020
@fingolfin fingolfin force-pushed the mh/remove-output-stack branch 3 times, most recently from 28da470 to 11c0b93 Compare October 12, 2020 10:12
@coveralls
Copy link

coveralls commented Oct 12, 2020

Coverage Status

Coverage decreased (-0.0004%) to 93.728% when pulling f762396 on fingolfin:mh/remove-output-stack into d4781bb on gap-system:master.

Comment on lines +1874 to +1879
TypOutputFile * output = IO()->Output;
if (!output)
ErrorMayQuit("SET_PRINT_FORMATTING_STDOUT called while no output is opened\n", 0, 0);
while (output->prev)
output = output->prev;
output->format = (val != False);
Copy link
Member Author

Choose a reason for hiding this comment

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

As an accidental byproduct, this change also fixes #1308

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Looks fine except perhaps addressing that pre-existing typo.

src/io.h Outdated Show resolved Hide resolved
@fingolfin fingolfin force-pushed the mh/remove-output-stack branch 2 times, most recently from b4e50e0 to 8a69634 Compare October 15, 2020 21:04
Instead of pre-allocating a fixed number of TypOutputFile instances on each
thread, we allocate the required storage dynamically on the stack. This
arguably makes it easier to reason about the global state of GAP.

It also enables future simplifications and improvements, e.g. to fix issues
were switching between stdout and errout breaks tracking of the output state,
which leads to GAP expecting resp. inserting linebreaks in the wrong spots.
@fingolfin fingolfin added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 15, 2020
@fingolfin fingolfin merged commit e62d395 into gap-system:master Oct 16, 2020
@ThomasBreuer ThomasBreuer self-assigned this Feb 16, 2021
@ThomasBreuer ThomasBreuer added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@ThomasBreuer ThomasBreuer removed their assignment Feb 16, 2021
@fingolfin fingolfin deleted the mh/remove-output-stack branch October 28, 2021 09:20
@fingolfin fingolfin changed the title kernel: remove output stream stack Fix a long standing bug which prevented SetPrintFormattingStatus( "*stdout*", false ); from working as expected Aug 17, 2022
@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SetPrintFormattingStatus( "*stdout*", false ); does not quite work
4 participants