Rename QUIT_GAP, FORCE_QUIT_GAP and GAP_EXIT_CODE to QuitGap, ForceQuitGap and GapExitCode (the old names remain available as synonyms)
#3862
Conversation
|
We could decide that QUIT_GAP -> QuitGap is OK, but leave FORCE_QUIT_GAP as all caps, as that function is "less user safe"... |
ChrisJefferson
added
kind: enhancement
| else | ||
| QUIT_GAP(1); | ||
| QuitGap(1); |
There was a problem hiding this comment.
Independent of this PR: looking at this PR, I discovered SetExitValue and learned that one could rewrite the above code as QuitGap(testTotalFailures = 0);. But I am not aware of any code using that. So, is it worth keeping this complication? If so, shouldn't we then actually use it here and elsewhere (perhaps not as part of this PR)?
On the other hand, it seems to me that code like QuitGap(testTotalFailures = 0);
There was a problem hiding this comment.
So, I have used SetExitValue(1), so if GAP exits in the middle of my program (say because of an error), the exit value ends up set to a non-zero value.
Using QUIT_GAP(boolean), I don't remember ever seeing. We could probably remove it.
There was a problem hiding this comment.
Looking at packages, there are a couple of QUIT_GAP(), but no passing in of booleans I can see.
There was a problem hiding this comment.
I agree that SetExitValue is useful to have, yes.
| { | ||
| if ( LEN_LIST(args) == 0 ) { | ||
| SystemErrorCode = 0; | ||
| } | ||
| else if ( LEN_LIST(args) != 1 | ||
| || !SetExitValue(ELM_PLIST(args, 1) ) ) { | ||
| ErrorQuit( "usage: QUIT_GAP( [ <return value> ] )", 0, 0); | ||
| ErrorQuit( "usage: QuitGap( [ <return value> ] )", 0, 0); |
There was a problem hiding this comment.
BTW, I am not aware of any code calling QuitGap or ForceQuitGap without an exit code. Are you? What would be a use case?
There was a problem hiding this comment.
I don't think I've done it myself. I suppose someone might want to just quit gap, but asking for a return value doesn't feel unreasonable.
ChrisJefferson
force-pushed
the
quit_gap
branch
from
4f5cae9
to
c282b74
Compare
src/gap.c
Outdated
| ** | ||
| */ | ||
|
|
||
| static Obj FuncGAP_EXIT_CODE(Obj self, Obj code) | ||
| static Obj FuncGapExitCode(Obj self, Obj code) |
There was a problem hiding this comment.
So tangentially related to this PR (which BTW needs to be rebased, then it could be merged): I just had need to query the exit code, and there simply is no way to do this. However, the natural name for a function to do this is taken... Can we either rename this to SetGapExitCode (and then later we can can add GapExitCode or GetGapExitCode to query it); or else change this function to return the exit code, and then also to allow calling it with 0 args, to just get the exit code w/o changing it?
There was a problem hiding this comment.
Or how about SetExitCode -- or for that matter, SetExitValue which would match the kernel function (or conversely: rename the kernel function to match the function here...)
|
I took the liberty of rebasing this PR. |
src/gap.c
Outdated
| ** | ||
| */ | ||
|
|
||
| static Obj FuncGAP_EXIT_CODE(Obj self, Obj code) | ||
| static Obj FuncGapExitCode(Obj self, Obj code) |
There was a problem hiding this comment.
Or how about SetExitCode -- or for that matter, SetExitValue which would match the kernel function (or conversely: rename the kernel function to match the function here...)
fingolfin
changed the title
ThomasBreuer
added
release notes: added
fingolfin
changed the title
QUIT_GAP, FORCE_QUIT_GAP and GAP_EXIT_CODE to QuitGap, ForceQuitGap and GapExitCode (the old names remain available as synonyms)
fingolfin
added
kind: removal or deprecation
A usual gap rule is CamelCase functions are for users, and ALL_CAPS are internal.
I think enough people are now using QUIT_GAP, FORCE_QUIT_GAP and GAP_EXIT_CODE that they should be moved to CamelCase (obviously leaving the old ALL_CAPS versions as obsolete).