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

debug: implement treeRepr() for PNode #191

Merged
merged 1 commit into from
Jan 22, 2022

Conversation

haxscramper
Copy link
Collaborator

@haxscramper haxscramper commented Jan 21, 2022

debug: implement treeRepr() for PNode

  • ADD implementation of the treeRepr() proc for the PNode, placed in
    the new utils/astrepr file. New file was added to avoid mixing "ast
    algorithms" with "debug printing".
  • ADD New overload for addInDebugUtils - PNode -> PSym trace. This was
    necessary to write an example on fixing a specific bug.
  • FIX/ADD return support for AST printing output in the sem trace - it was
    present before structured report refactor but then got removed. Not it is
    back again.
  • REMOVE old implementaiton of the debug calls - their functionality is
    fully replaced by the new treeRepr()
  • REMOVE implementation of the treeToYaml converter - their functionality
    has been fully replaced with the treeRepr() which also has almost
    identical representation. They were not used anywhere in the compiler or
    tests, so it would be easy to assume they were not used for anything
    besides debugging.
  • DOC add example of the updated sem tracer. Move the 'compiler debugging'
    documentation to a separate file - it is already pretty long and does not
    really fit into 'compiler architecture' that is intern.rst

Remaining todo items:

  • Finish documentation comments for the AST repr
  • Write adequate commit message
  • Add missing printing information
  • treeRepr for the PSym and PType
  • Declaration location for nodes
  • Restore missing functionality of the tree printer for the sem tracer reports - treeRepr, astRepr and others can be used there.

Sorry, something went wrong.

saem
saem previously requested changes Jan 21, 2022
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

One typo, looks good to me.

doc/intern.rst Outdated Show resolved Hide resolved
compiler/sem/semexprs.nim Outdated Show resolved Hide resolved
@haxscramper haxscramper force-pushed the cleanup-astalgo branch 5 times, most recently from 7055da7 to ecbc5a6 Compare January 22, 2022 17:50
@haxscramper haxscramper dismissed saem’s stale review January 22, 2022 17:52

No longer valid, all suggestions have been fixed.

@haxscramper
Copy link
Collaborator Author

ready to be merged

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

@haxscramper did a review, sorry I'm a little off today. But I hope the feedback is useful. Let me know if you have any concerns.

This is incredible by the way, we can teach people now. It's bonkers how fast we got here.

compiler/ast/ast.nim Show resolved Hide resolved
compiler/utils/astrepr.nim Show resolved Hide resolved
compiler/utils/astrepr.nim Outdated Show resolved Hide resolved
compiler/utils/tree_repr_doc.rst Outdated Show resolved Hide resolved
compiler/front/cli_reporter.nim Outdated Show resolved Hide resolved
doc/debug.rst Outdated

Both *compiler* and and your file must be compiled with defines - main
compiler requires `nimDebugUtils` (this guard exists to avoid heavy
performance hits), and your test compiler need to be run with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
performance hits), and your test compiler need to be run with
performance hits), and your file needs to be compiled with

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, it means that main compiler compiles tests compiler, there is no error. Previous suggestion removing main is also incorrect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how to word this properly, but considering we are compiling the compiler to compile the file there is bound to be some weird phrasing

Copy link
Collaborator

@saem saem Jan 22, 2022

Choose a reason for hiding this comment

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

Then I need to follow through the instructions again, because I'm not getting the mechanics.

I would think we'd want to flip switches (defines) that would produce a test compiler that'll output all the things. Then when we go to use that test compiler we provide more switches (defines) that would control the output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is:

  1. Create a compiler that knows to trace (compile a test compiler with defines)
  2. Use the test compiler created in the prior step to compile your program with tracing parameters to control the output

doc/debug.rst Outdated Show resolved Hide resolved
doc/debug.rst Outdated Show resolved Hide resolved
doc/debug.rst Outdated Show resolved Hide resolved
lib/experimental/colortext.nim Show resolved Hide resolved
doc/debug.rst Outdated
Comment on lines 67 to 71
Both *compiler* and your file must be compiled with defines - *main*
compiler requires `nimDebugUtils` (this guard exists to avoid heavy
performance hits), and your *test* compiler need to be run with
`nimCompilerDebugCalltrace` to control what exact parts of the default
debugger will be executed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Both *compiler* and your file must be compiled with defines - *main*
compiler requires `nimDebugUtils` (this guard exists to avoid heavy
performance hits), and your *test* compiler need to be run with
`nimCompilerDebugCalltrace` to control what exact parts of the default
debugger will be executed.
Both *compiler* and your file must be compiled with defines. The
compiler must be built with `nimDebugUtils` (this guard avoids heavy
performance hits). Your file must be compiled with
`nimCompilerDebugCalltrace` to control which exact parts of the
default debugger will be executed.

- ADD implementation of the `treeRepr()` proc for the `PNode`, placed in
  the new `utils/astrepr` file. New file was added to avoid mixing "ast
  algorithms" with "debug printing".
- ADD New overload for `addInDebugUtils` - `PNode -> PSym` trace. This was
  necessary to write an example on fixing a specific bug.
- FIX/ADD return support for AST printing output in the sem trace - it was
  present before structured report refactor but then got removed. Not it is
  back again.
- REMOVE old implementaiton of the `debug` calls - their functionality is
  fully replaced by the new `treeRepr()`
- REMOVE implementation of the `treeToYaml` converter - their functionality
  has been fully replaced with the `treeRepr()` which also has almost
  identical representation. They were not used anywhere in the compiler or
  tests, so it would be easy to assume they were not used for anything
  besides debugging.
- DOC add example of the updated sem tracer. Move the 'compiler debugging'
  documentation to a separate file - it is already pretty long and does not
  really fit into 'compiler architecture' that is `intern.rst`
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

bors r+

🎉

@bors
Copy link
Contributor

bors bot commented Jan 22, 2022

Build succeeded:

@bors bors bot merged commit c58bb08 into nim-works:devel Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants