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

Add a callback to FuncJUMP_TO_CATCH #1690

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

markuspf
Copy link
Member

@markuspf markuspf commented Sep 7, 2017

This is another bit of the GAP dynamic library code. Again without tests
and without a guarantee that this will work in HPC-GAP

@markuspf markuspf added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Sep 7, 2017
src/gapstate.h Outdated
@@ -148,6 +148,7 @@ typedef struct GAPState {
Obj ErrorLVars0; // the initial ErrorLVars value, i.e. for the lvars were the break occurred
Obj ErrorLVars; // ErrorLVars as modified by DownEnv / UpEnv
Int ErrorLLevel; // record where on the stack ErrorLVars is relative to the top, i.e. ErrorLVars0
void (*JumpToCatchFunc)(); // Error catching function for GAP shared library
Copy link
Member

Choose a reason for hiding this comment

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

This JumpToCatchFunc is a bit mysterious, and of course currently never set. And this comment doesn't really help ("shared library? what shared library?")

Perhaps "Callback invoked by FuncJUMP_TO_CATCH" would be more helpful here, and then a somewhat longer explanation in "FuncJUMP_TO_CATCH", like: "Optionally invoke a callback just before we jump. This is not used by GAP itself, but exists for use in code linking against GAP."

@codecov
Copy link

codecov bot commented Sep 7, 2017

Codecov Report

Merging #1690 into master will decrease coverage by 3.32%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1690      +/-   ##
==========================================
- Coverage   69.72%   66.39%   -3.33%     
==========================================
  Files         482      569      +87     
  Lines      254511   310151   +55640     
  Branches        0    12029   +12029     
==========================================
+ Hits       177453   205928   +28475     
- Misses      77058   101427   +24369     
- Partials        0     2796    +2796
Impacted Files Coverage Δ
src/gap.c 57.1% <66.66%> (-6.41%) ⬇️
src/hpc/tls.c 57.69% <0%> (-42.31%) ⬇️
lib/obsolete.gd 28.35% <0%> (-32.94%) ⬇️
lib/init.g 45.28% <0%> (-25.23%) ⬇️
src/objccoll-impl.h 69.07% <0%> (-22.6%) ⬇️
src/sctable.c 47.79% <0%> (-22.58%) ⬇️
src/hpc/aobjects.h 62.5% <0%> (-22.12%) ⬇️
src/hpc/guards.h 60% <0%> (-20.56%) ⬇️
src/intobj.h 79.59% <0%> (-20.41%) ⬇️
src/funcs.h 80% <0%> (-20%) ⬇️
... and 383 more

@olexandr-konovalov olexandr-konovalov added the gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall label Sep 7, 2017
@markuspf
Copy link
Member Author

Not ready.

@markuspf markuspf closed this Oct 24, 2017
@markuspf markuspf reopened this Feb 13, 2018
@markuspf markuspf force-pushed the jump-to-catch branch 2 times, most recently from 2186e56 to 6d84bca Compare February 13, 2018 11:30
@markuspf
Copy link
Member Author

As part of an effort with @sebasguts to get libgap into master, I resurrected this, and rebased it on current master.

Added a longer comment about what the added callback function is supposed to do.

I am not sure this is the correct way to handle errors for a library in the long run, but at least for now this works (and is compatible with whatever Sage wants to do). If anyone has a better idea (that ideally does not require a complete rewrite of error handling right now), I am all ears.

@markuspf markuspf requested review from fingolfin and ChrisJefferson and removed request for ChrisJefferson February 13, 2018 11:35
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.

I don't mind this much, now that there is a comment explaining what it's for; and with the implicit understanding that we just keep this around for the time being, because it is pragmatic, but that if we ever refactor error handling, we can replace it with something better (if we coordinate this with SageMath, it shouldn't hurt them either, after all).

src/gap.c Outdated
}

/* TL: UInt UserHasQuit; */
/* TL: UInt UserHasQUIT; */
UInt SystemErrorCode;
Copy link
Member

Choose a reason for hiding this comment

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

These three lines look like a merge artifact -- remove?

src/gapstate.h Outdated
@@ -123,6 +123,9 @@ typedef struct GAPState {
Obj BaseShellContext;
Obj ErrorLVars; // ErrorLVars as modified by DownEnv / UpEnv
Int ErrorLLevel; // record where on the stack ErrorLVars is relative to the top, i.e. BaseShellContext
void (*JumpToCatchFunc)(); // Callback that is called in FuncCALL_TO_CATCH,
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 called in FuncJUMP_TO_CATCH, not CALL_...

Would you mind renaming the callback to JumpToCatchCallback? I find that slightly more helpful when reading the code. But it's probably subjective, so shrug

@markuspf markuspf force-pushed the jump-to-catch branch 2 times, most recently from fe65fd3 to ed3047d Compare February 13, 2018 12:05
This is another bit of the GAP dynamic library code. Again without tests
and without a guarantee that this will work in HPC-GAP
@ChrisJefferson ChrisJefferson merged commit 9391c12 into gap-system:master Feb 19, 2018
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jun 15, 2018
@markuspf markuspf deleted the jump-to-catch branch June 29, 2018 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants