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

Reduce number of functions and macros which return pointers into GAP objects #2344

Merged
merged 11 commits into from
Apr 10, 2018

Conversation

fingolfin
Copy link
Member

More specifically, avoid returning CSTR_STRING(foo) for GAP strings foo, as this is very prone to introduce bugs triggered by a garbage collection in the wrong moment.

Some thoughts:

  • the new %H added to Pr() actually replaced all instances of %I, so we could drop the old %I and rename %H to %I, thus reducing the final diff in this PR quite a bit.
  • I checked carefully that none of the functions changed and/or renamed by this PR are used by any packages (with a tiny exception for ParGAP, which wraps NAME_LVAR, but only to aid debuggers like GDB/LLDB; this is trivial to fix, but I consider ParGAP unmaintained at this point and thus don't care about breaking it further).

Closes #2337

@codecov
Copy link

codecov bot commented Apr 7, 2018

Codecov Report

Merging #2344 into master will increase coverage by <.01%.
The diff coverage is 84.4%.

@@            Coverage Diff             @@
##           master    #2344      +/-   ##
==========================================
+ Coverage   73.07%   73.07%   +<.01%     
==========================================
  Files         480      480              
  Lines      246622   246620       -2     
==========================================
- Hits       180213   180212       -1     
+ Misses      66409    66408       -1
Impacted Files Coverage Δ
src/gvars.h 100% <ø> (ø) ⬆️
src/vars.h 100% <ø> (ø) ⬆️
src/gap.c 90.63% <ø> (ø) ⬆️
src/calls.h 95.74% <ø> (ø) ⬆️
src/hpc/serialize.c 8.35% <0%> (ø) ⬆️
src/hpc/aobjects.c 68.27% <0%> (ø) ⬆️
src/intrprtr.c 94.05% <100%> (ø) ⬆️
src/exprs.c 93.6% <100%> (ø) ⬆️
src/precord.c 96.31% <100%> (-0.02%) ⬇️
src/error.c 77.19% <100%> (ø) ⬆️
... and 12 more

@ChrisJefferson
Copy link
Contributor

Thanks, this is better than my changes were. I'll trust you checked all the packages for possible breakage! (Although tests will show us fairly soon after merging).

After this is merged and settled, I will do another full pass of memory checking, just to make sure nothing was missed.

@fingolfin
Copy link
Member Author

Yes, I checked for conflicts with packages rather extensively: in my worktree, I have both a "bootstrap" pkg dir (i.e. with all the currently distributed packages, resp. their latest versions ready for distribution), and also another pkg dir with clones of all packages in gap-packages, plus a few more hosted elsewhere. I then used ripgrep to grep for any function or macro whose return value I modified, to see if any package uses them. That yielded only one hit: pargap has a C function which calls NAMI_FUNC, but (a) it's in an #if 0 block, and (b) a comment explains that it is meant to help debugging (presumably in gdb/lldb) -- that comment also expresses the wish that GAP itself should do that. Well, good news, we do now, so that part of pargap is obsolete. ;-)

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Apr 10, 2018
@fingolfin fingolfin merged commit 8de5914 into gap-system:master Apr 10, 2018
@fingolfin fingolfin deleted the mh/less-cstr branch April 10, 2018 13:33
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jul 31, 2018
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.

2 participants