Add WhereWithVars - a more verbose Where #3564

ChrisJefferson · 2019-07-14T08:22:51Z

Description

This adds an extended version of Where, called WhereWithVars, which adds the values of all arguments and locals to the backtrace given in a breakloop. This is a tidied up version of some code I've had use privately for a while.

I didn't just add an argument to 'Where', as in some places 'Where' is replaced.

Text for release notes

Add 'WhereWithVars', an extended version of Where which prints the values of all arguments and locals.

codecov · 2019-07-14T09:08:27Z

Codecov Report

Merging #3564 into master will increase coverage by 0.79%.
The diff coverage is 17.02%.

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
+ Coverage   83.73%   84.52%   +0.79%     
==========================================
  Files         695      696       +1     
  Lines      344884   345125     +241     
==========================================
+ Hits       288789   291727    +2938     
+ Misses      56095    53398    -2697

Impacted Files	Coverage Δ
lib/error.g	`39.14% <17.02%> (-4.19%)`	⬇️
src/gasman.c	`86.28% <0%> (-3.66%)`	⬇️
src/iostream.c	`65.15% <0%> (-0.76%)`	⬇️
src/gap.c	`81.35% <0%> (-0.37%)`	⬇️
lib/debug.g	`23.57% <0%> (ø)`
lib/algrep.gi	`80.46% <0%> (+0.04%)`	⬆️
lib/cyclotom.gi	`79.56% <0%> (+0.08%)`	⬆️
lib/ratfun.gi	`90.42% <0%> (+0.08%)`	⬆️
src/plist.c	`95.06% <0%> (+0.08%)`	⬆️
lib/meataxe.gi	`85.11% <0%> (+0.08%)`	⬆️
... and 125 more

coveralls · 2019-07-14T09:09:47Z

Coverage decreased (-0.009%) to 84.307% when pulling a70e933 on ChrisJefferson:wherewithvars into ac5c014 on gap-system:master.

fingolfin

Overall, I like this! Some minor questions, though

fingolfin · 2019-07-14T13:36:29Z

tst/testspecial/backtrace.g.out

+od; at *stdin*:24
+with local state:
+-Arguments:
+-Locals:


Why is there a minus sign in front of these strings? Or is that supposed to be a list? Then I'd suggest to add a space after the -.

(For the following, please keep in mind that I am German: these are optional suggestions, not concealed demands :-)

Perhaps this should be indented by a space or two (the "called from" is also indented, after all); and perhaps replace "Arguments" by "arguments" and "Locals" by "locals", which seems to me to better match with the surrounding text?

And perhaps when there are no arguments, one could print - arguments: <none> ? Same for locals, I guess?

I played around a little with the formatting, I'm happy for other suggestions, and couldn't come up with anything I loved.

A nicer handling of 'none' definitely seems like a nice idea.

tst/testspecial/backtrace.g.out

fingolfin · 2019-07-14T14:07:00Z

lib/error.g

+BIND_GLOBAL("PRETTY_PRINT_VARS", function(context)
+    local vars, i;
+    vars := ContentsLVars(context);
+    if IsRecord(vars) then


A matter of taste, but there'd be less need for indentation if you used if not IsRecord(vars) then return; fi;.

lib/error.g

fingolfin · 2019-07-14T14:11:12Z

doc/ref/mloop.xml

 <Description>
 <Index Subkey="GAP3 name for Where">Backtrace</Index>
 <Index>Stack trace</Index>
 shows the last <A>nr</A> commands on the execution stack during whose execution
 the error occurred. If not given, <A>nr</A> defaults to 5. (Assume, for the
 following example, that after the last example <Ref Func="OnBreak"/>
-has been set back to its default value.)
+has been set back to its default value.). <Ref Func="WhereWithVars"/> also
+shows the function arguments and local variables in each stack frame


Suggested change

shows the function arguments and local variables in each stack frame

shows the function arguments and local variables in each stack frame.

The term "stack frame" is not actually defined in the manual, is it? In that case, it might be better to either define it, or avoid it here.

ChrisJefferson · 2019-07-14T21:50:42Z

i've tried overhauling the output based on printing records. it looks a bit awkward as PrintTo(ERROR_OUTPUT,...) doesn't preserve the indentation level.

tst/testspecial/backtrace.g.out

fingolfin

The output looks great to me now, thanks! This is a very useful feature.

However, there is something I don't understand about the line break hints in the implementation.

doc/ref/mloop.xml

fingolfin · 2019-07-15T09:22:05Z

lib/error.g

+    vars := ContentsLVars(context);
+    if IsRecord(vars) then
+        argcount := ABS_RAT(NumberArgumentsFunction(vars.func));
+        PrintTo(ERROR_OUTPUT, "\>\>\n\<\<arguments:");


That doesn't make sense to me. Why would you decrease the hint level with \>\> at the start, and then after the newline increase it again? Is that to counteract some line break hints in the caller? But that would seem rather fragile, and at least should be documented.

So, \>\> increases the hint level (at least, it causes an indent). The reason I write this strangely is because (as I mention below, but I should pull the comment up) there is a long-standing bug in PrintTo(ERROR_OUTPUT, (and a related places in printing) where the indentation level gets reset on each print call.

Therefore I'm making sure I do all the indentation I want in a single call to PrintTo is balanced (which I believe I've achieved, unless I'm missing something), so that at the moment I end up with the output I want, and if PrintTo is ever fixed, the output won't change.

I'll write a longer comment pointing this out at the top of the function

Thanks for the explanation, I mixed up the meaning of \< and \>.

And: Ah yeah, I remember that bug: the problem is that for each such PrintTo call, we "open" stderr, print to it, and close it -- but the output state for the stdout and stderr stream are not shared, but arguably should be, at least if both are known to map to the same device. We probably should have an issue for that (perhaps we already do, I didn't check).

lib/error.g

fingolfin

LGTM, thank you!

fingolfin

Actually, I think there is still a typo in the manual -- at least I can't find a function Break anywhere?

fingolfin reviewed Jul 14, 2019

View reviewed changes

ChrisJefferson force-pushed the wherewithvars branch 2 times, most recently from f307260 to b777a6b Compare July 14, 2019 21:49

fingolfin reviewed Jul 14, 2019

View reviewed changes

tst/testspecial/backtrace.g.out Outdated Show resolved Hide resolved

ChrisJefferson force-pushed the wherewithvars branch from b777a6b to e8e9dde Compare July 15, 2019 08:18

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: library labels Jul 15, 2019

fingolfin reviewed Jul 15, 2019

View reviewed changes

ChrisJefferson force-pushed the wherewithvars branch from 1f7a92f to 3432010 Compare July 15, 2019 09:46

fingolfin approved these changes Jul 15, 2019

View reviewed changes

fingolfin requested changes Jul 15, 2019

View reviewed changes

Add WhereWithVars

a70e933

ChrisJefferson force-pushed the wherewithvars branch from 1c3871e to a70e933 Compare July 15, 2019 18:30

fingolfin approved these changes Jul 16, 2019

View reviewed changes

fingolfin merged commit b5373a3 into gap-system:master Jul 16, 2019

wucas 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 20, 2019

ChrisJefferson deleted the wherewithvars branch August 23, 2019 10:55

olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WhereWithVars - a more verbose Where #3564

Add WhereWithVars - a more verbose Where #3564

ChrisJefferson commented Jul 14, 2019 •

edited by wilfwilson

Loading

codecov bot commented Jul 14, 2019 •

edited

Loading

coveralls commented Jul 14, 2019 •

edited

Loading

fingolfin left a comment

fingolfin Jul 14, 2019

ChrisJefferson Jul 14, 2019

fingolfin Jul 14, 2019

fingolfin Jul 14, 2019

ChrisJefferson commented Jul 14, 2019

fingolfin left a comment

fingolfin Jul 15, 2019

ChrisJefferson Jul 15, 2019 •

edited

Loading

ChrisJefferson Jul 15, 2019

fingolfin Jul 15, 2019

fingolfin left a comment

fingolfin left a comment

	shows the function arguments and local variables in each stack frame
	shows the function arguments and local variables in each stack frame.

Add WhereWithVars - a more verbose Where #3564

Add WhereWithVars - a more verbose Where #3564

Conversation

ChrisJefferson commented Jul 14, 2019 • edited by wilfwilson Loading

Description

Text for release notes

codecov bot commented Jul 14, 2019 • edited Loading

Codecov Report

coveralls commented Jul 14, 2019 • edited Loading

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jul 14, 2019

Choose a reason for hiding this comment

ChrisJefferson Jul 14, 2019

Choose a reason for hiding this comment

fingolfin Jul 14, 2019

Choose a reason for hiding this comment

fingolfin Jul 14, 2019

Choose a reason for hiding this comment

ChrisJefferson commented Jul 14, 2019

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jul 15, 2019

Choose a reason for hiding this comment

ChrisJefferson Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

ChrisJefferson Jul 15, 2019

Choose a reason for hiding this comment

fingolfin Jul 15, 2019

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

ChrisJefferson commented Jul 14, 2019 •

edited by wilfwilson

Loading

codecov bot commented Jul 14, 2019 •

edited

Loading

coveralls commented Jul 14, 2019 •

edited

Loading

ChrisJefferson Jul 15, 2019 •

edited

Loading