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

Make "last" (and similar variables) read-only #3226

Merged
merged 2 commits into from
Jan 27, 2019

Conversation

ChrisJefferson
Copy link
Contributor

This stops users writing to last, time, and some other variables, by making them read-only.

It adds a method to write to these variables, UpdateStat, mainly for use in demo.g.

I realise this might break some code. However, I did have someone had a had-to-track down bug where they were using last as a variable themselves, and were then confused when they got stupid answers out, so I think it is worth changing.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you, this seems like a very sensible thing to me. However, I have various nitpicks.

lib/demo.g Outdated
last := result[2];
UpdateStat("last3", last2);
UpdateStat("last2", last);
UpdateStat("last", result[2]);
Copy link
Member

Choose a reason for hiding this comment

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

Since you are touching this code anyway, perhaps you'd also be willing to teach it to update memory_allocated, too? Also perfectly fine if you don't want to, of course!

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've never actually used this code, and don't even know how it should be used, so I'd prefer to leave it. I've just noticed it has no tests, no documentation, and isn't linked to from anywhere?

Copy link
Member

Choose a reason for hiding this comment

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

It is used like this: you take a file and put a bunch of inputs in it, each on its own line. Then you run Demonstration on this; whenever you press a key, the next input line is read and executed. The result then looks something like this (where each demo> appeared only after a key was pressed):

gap> Demonstration("demo.g");

Start of demonstration.

demo> 1+1;
2
demo> Factorial(10);
3628800
demo>
End of demonstration.

gap>

Updating last, last2, etc. only is of interest if the demo code references them. So if one wants to write a demo that showcases those, and also memory_allocated, then it'd be nice to have memory_allocated. But this is nothing this PR needs to do.

I've long wanted to give this code some love, and improve it; e.g. instead of always going to the next command upon any key press, I think only "return" on an empty line should do that; otherwise, it'd be nice if one could show the next command w/o executing it, and also edit it; and other stuff. I think other people implemented such demo modes for other languages, and they can be very useful for classes, presentations during talks etc.

src/gap.c Outdated Show resolved Hide resolved
src/gap.h Outdated Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
src/scanner.c Outdated Show resolved Hide resolved
src/gap.c Outdated Show resolved Hide resolved
src/gap.c Outdated Show resolved Hide resolved
src/gap.c Outdated Show resolved Hide resolved
src/gap.h Outdated Show resolved Hide resolved
doc/ref/mloop.xml Outdated Show resolved Hide resolved
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jan 23, 2019
src/gap.c Outdated Show resolved Hide resolved
@ChrisJefferson ChrisJefferson force-pushed the protect-last branch 3 times, most recently from 95c36c5 to 7d491bc Compare January 23, 2019 18:41
@ChrisJefferson
Copy link
Contributor Author

I think all issues are now fixed.

@codecov
Copy link

codecov bot commented Jan 24, 2019

Codecov Report

Merging #3226 into master will decrease coverage by <.01%.
The diff coverage is 62.16%.

@@            Coverage Diff             @@
##           master    #3226      +/-   ##
==========================================
- Coverage   84.75%   84.75%   -0.01%     
==========================================
  Files         689      689              
  Lines      336391   336411      +20     
==========================================
+ Hits       285107   285116       +9     
- Misses      51284    51295      +11
Impacted Files Coverage Δ
src/gvars.h 100% <ø> (ø) ⬆️
lib/init.g 83.3% <ø> (-0.04%) ⬇️
src/streams.c 72.25% <100%> (-0.09%) ⬇️
src/gvars.c 86.04% <100%> (+0.05%) ⬆️
src/gap.c 81.14% <53.33%> (-1.42%) ⬇️
src/hpc/threadapi.c 46.82% <0%> (+0.04%) ⬆️
src/objset.c 85.35% <0%> (+0.22%) ⬆️

@coveralls
Copy link

coveralls commented Jan 24, 2019

Coverage Status

Coverage increased (+0.03%) to 84.677% when pulling c5b6027 on ChrisJefferson:protect-last into 211faf8 on gap-system:master.

A very subtle mistake in GAP is for users to use the variables
'last' or 'time', as GAP automatically rewrites these variables
after every statement. Add a special function 'UpdateStat' for
changing these variables, so we can disable 'last := x;'
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Perfect, thanks! I am waiting with the merge a bit, so that the PR adding C++ coverage support can finish first, as otherwise codecov will probably once again be super confused about what it displays sigh

@ChrisJefferson
Copy link
Contributor Author

Fixes #338 ( mentioned here so it will get closed)

@fingolfin
Copy link
Member

That doesn't work, you have to add the "fixes" to the PR description

@fingolfin fingolfin merged commit b72e418 into gap-system:master Jan 27, 2019
@ChrisJefferson ChrisJefferson deleted the protect-last branch February 27, 2019 18:02
@DominikBernhardt DominikBernhardt 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 Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants