-
Notifications
You must be signed in to change notification settings - Fork 161
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
New command line option --alwaystrace #2551
New command line option --alwaystrace #2551
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2551 +/- ##
==========================================
- Coverage 75.12% 74.9% -0.22%
==========================================
Files 478 478
Lines 242221 246757 +4536
==========================================
+ Hits 181965 184834 +2869
- Misses 60256 61923 +1667
|
4361ace
to
b042e10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'm sorry, but I really don't like the naming here, it's confusing how it relates to -T.
Maybe make --nobreakloop just the long name for -T, then add --backtraceonbreak , which modifies --nobreakloop/-T to keep the backtrack?
b042e10
to
85c24bb
Compare
I improved the naming (I hope). The new option is now called |
Out of interest, do you want to merge this then implement the functionality (which would be a little unusual), or carry on working on the function in this PR? |
I want to finish this over the next few days. :) |
5207e21
to
5e33302
Compare
I added a new commit which implements the new behaviour. I'll have to think about whether this is correct tomorrow again. Still missing: documentation of the new global variable |
5e33302
to
973ff34
Compare
973ff34
to
c378bac
Compare
Let's see whether tests pass on travis. :) |
lib/error.g
Outdated
# start an interactive break loop or whether we use JUMP_TO_CATCH | ||
# to exit this function. | ||
# BreakOnError is defined by the `-T` command line flag in init.g. | ||
doBreakLoop := BreakOnError and not QUITTING; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BreakOnError
is initialised depending on whether (and how many times?) -T
is given on the command line, the user can (unfortunately) set it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is quite fortunate that the user can do this, it has been crucial for some code I wrote, esp. for testing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll change it to initialize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unfortunate bit is that the user can set this to, say, a function which makes GAP crash when an error is hit.
lib/error.g
Outdated
doBreakLoop := BreakOnError and not QUITTING; | ||
# If we do not start a break loop, the variables SilentErrors and | ||
# AlwaysPrintTracebackOnError determine whether any messages or | ||
# respectively tracebacks are printed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll expand on that. 👍
lib/error.g
Outdated
if IsHPCGAP then | ||
LastErrorMessage := ""; | ||
lastErrorStream := OutputTextString(LastErrorMessage, true); | ||
for x in earlyMessage do | ||
PrintTo(lastErrorStream, x); | ||
od; | ||
# FIXME: Also make HPCGAP work with AlwaysPrintTracebackOnError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What needs to be done to achieve this? I see some code added for HPC-GAP, is this comment obsolete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't obsolete. IMO if AlwaysPrintTracebackOnError
is true, the traceback should also be put into LastErrorMessage
. To do this there needs to be a way to put the output of Where()
into lastErrorStream
.
I'll add a comment to the source.
lib/init.g
Outdated
@@ -248,10 +250,15 @@ CallAndInstallPostRestore( function() | |||
|
|||
if IsHPCGAP then | |||
BindThreadLocal( "BreakOnError", not GAPInfo.CommandLineOptions.T ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really relevant to this PR: We should comment here why these variables are thread local (I suppose we don't want a break look in a worker thread; I have had students trying to debug task-based code, which was a big pain becuase of no break loops or error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't merge this quite yet, I'd like to understand this PR properly first.
lib/error.g
Outdated
if SHOULD_QUIT_ON_BREAK() then | ||
FORCE_QUIT_GAP(1); | ||
fi; | ||
|
||
if IsBound(OnBreak) and IsFunction(OnBreak) then | ||
OnBreak(); | ||
fi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, so OnBreak
is now called beforethe test for
SHOULD_QUIT_ON_BREAK`? That's quite a change in semantics, one which I am quite concerned about. I'll need to look at your whole PR with a bit more time to understand what is going on.
(It might help if the refactoring did not happen at the same time as the changes in behavior -- note that to me, "refactoring" means a change which does not change behavior (or at most changes in a very minimalistic way, which then however should be documented in the commit message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right. I'll make separate commits out of that. Note to myself: never mix refactoring and behaviour changes in a single commit.
I realize that I forgot to check for SHOULD_QUIT_ON_BREAK()
in the if-clause that controls whether OnBreak()
is called. That change in semantics was not on purpose.
In the commit message, you write:
I can't judge if this is really code refactoring or not, as it is mixed with actual code changes, some of which match what you describe, but some I don't see reflected in your descriptions, so I wonder if they are intentational. If the refactoring was done in a separate commit, this would be far easier to evaluate. As to avoiding code duplication: Another technique in this case that might work is to put the code being duplicatated into an inner function (which thus has access to all local vars), and simply call that from all the places that need to call it. That's a change which is fairly easy to review, too. |
3251a8f
to
3071c3f
Compare
@fingolfin @markuspf I addressed your comments. I split the refactoring into a seperate commit and changed it so that it introduces new local functions. I think this makes this PR a lot clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now.
lib/error.g
Outdated
od; | ||
end; | ||
|
||
printEarlyTraceback := function(stream, context, printThisStatement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop context, printThisStatement arguments
lib/error.g
Outdated
for x in earlyMessage do | ||
PrintTo(lastErrorStream, x); | ||
od; | ||
printEarlyMessage(lastErrorStream, earlyMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you change the behaviour slightly, now Error,
is printed to lastErrorStream
(I am not sure if this matter for anything, just observing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dug a bit, and as far as I can tell, this only affects the TaskError()
function (and of course the LastErrorMessage
global). I'd still prefer if the redundant arguments were dropped, as they are rather "un-GAPish", but if this is merged with it, I can also fix it myself shrug
lib/error.g
Outdated
# Again, the default is to not print the rest of the traceback. | ||
# If AlwaysPrintTracebackOnError is true we do so anyways. | ||
if | ||
AlwaysPrintTracebackOnError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird line break, please merge the two preceding lines into one.
lib/error.g
Outdated
|
||
|
||
# Local functions that print the user feedback. | ||
printEarlyMessage := function(stream, earlyMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to pass earlyMessage
as an argument, it is available automatically in this closure. I.e. just drop the second argument here and in calls, no other change, and things will work just fine.
BindThreadLocal( "LastErrorMessage", "" ); | ||
else | ||
ASS_GVAR( "BreakOnError", not GAPInfo.CommandLineOptions.T ); | ||
ASS_GVAR( "SilentErrors", false ); | ||
ASS_GVAR( "AlwaysPrintTracebackOnError", GAPInfo.CommandLineOptions.alwaystrace ); | ||
ASS_GVAR( "SilentNonInteractiveErrors", false ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note a change request, just a remark: if the renaming of SilentErrors
to SilentNonInteractiveErrors
were in a separate commit, that'd make things even easier to review. But it's fine to keep it now as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed two commits which fix my complaints. Feel free to squash these commits into your other commits; or we can also just merge this PR as-is.
lib/error.g
Outdated
for x in earlyMessage do | ||
PrintTo(lastErrorStream, x); | ||
od; | ||
printEarlyMessage(lastErrorStream, earlyMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dug a bit, and as far as I can tell, this only affects the TaskError()
function (and of course the LastErrorMessage
global). I'd still prefer if the redundant arguments were dropped, as they are rather "un-GAPish", but if this is merged with it, I can also fix it myself shrug
817589e
to
1a9f76c
Compare
This commit introduces some local functions to print the different parts of the user feedback. It also adds some source comments and renames the undocumented variable SilentErrors to SilentNonInteractiveErrors. The refactoring is done to e.g. also print a traceback from other parts of ErrorInner without having to duplicate the printing code.
The option --alwaystrace sets a new global variable AlwaysPrintTracebackOnError. If set to true `OnBreak()` is always called before exiting `ErrorInner`.
1a9f76c
to
c80941d
Compare
I've rebased the commits. I squashed |
Also introduces a new global variable
AlwaysPrintTracebackOnError
.Funtionality and documentation yet to be implemented.