Skip to content
This repository was archived by the owner on Mar 25, 2025. It is now read-only.

Refactor nmodl ... blame --line N.#1265

Merged
1uc merged 1 commit into
masterfrom
1uc/improve-blame
May 22, 2024
Merged

Refactor nmodl ... blame --line N.#1265
1uc merged 1 commit into
masterfrom
1uc/improve-blame

Conversation

@1uc

@1uc 1uc commented May 15, 2024

Copy link
Copy Markdown
Collaborator

The revised format only prints the first trace, before entering the CodePrinter. The new output (for a particularly verbose case) is:

$ nmodl art_toggle.mod blame --line 264
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.cpp", line 546, in print_statement_block
       545:              !statement->is_mutex_unlock() && !statement->is_protect_statement()) {
    >  546:              printer->add_indent();
       547:          }
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_coreneuron_cpp_visitor.cpp", line 2424, in print_net_send_call
      2423:      if (info.artificial_cell) {
    > 2424:          printer->fmt_text("artcell_net_send(&{}, {}, {}, nt->_t+", tqitem, weight_index, pnt);
      2425:      } else {
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.hpp", line 1387, in print_vector_elements<std::shared_ptr<nmodl::ast::Expression> >
      1386:      for (auto iter = elements.begin(); iter != elements.end(); iter++) {
    > 1387:          printer->add_text(prefix);
      1388:          (*iter)->accept(*this);
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.cpp", line 639, in visit_double
       638:  void CodegenCppVisitor::visit_double(const Double& node) {
    >  639:      printer->add_text(format_double_string(node.get_value()));
       640:  }
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.hpp", line 1390, in print_vector_elements<std::shared_ptr<nmodl::ast::Expression> >
      1389:          if (!separator.empty() && !nmodl::utils::is_last(iter, elements)) {
    > 1390:              printer->add_text(separator);
      1391:          }
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.hpp", line 1387, in print_vector_elements<std::shared_ptr<nmodl::ast::Expression> >
      1386:      for (auto iter = elements.begin(); iter != elements.end(); iter++) {
    > 1387:          printer->add_text(prefix);
      1388:          (*iter)->accept(*this);
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.cpp", line 639, in visit_double
       638:  void CodegenCppVisitor::visit_double(const Double& node) {
    >  639:      printer->add_text(format_double_string(node.get_value()));
       640:  }
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_coreneuron_cpp_visitor.cpp", line 2433, in print_net_send_call
      2432:      print_vector_elements(arguments, ", ");
    > 2433:      printer->add_text(')');
      2434:  }
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.cpp", line 550, in print_statement_block
       549:          if (need_semicolon(*statement)) {
    >  550:              printer->add_text(';');
       551:          }
    Source: "/home/lucg/git/bbp/nmodl/src/codegen/codegen_cpp_visitor.cpp", line 553, in print_statement_block
       552:          if (!statement->is_mutex_lock() && !statement->is_mutex_unlock()) {
    >  553:              printer->add_newline();
       554:          }

It also refactors the code to prepare for injecting a detailed blame printer, when needed.

@1uc
Refactor nmodl ... blame --line N.
bb923fd 
The revised format only prints the first trace, before entering the
`CodePrinter`.

It also refactors the code to prepare for injecting a detailed blame
printer, when needed.
bbpadministrator pushed a commit to BlueBrain/nmodl-references that referenced this pull request May 15, 2024
References for 'BlueBrain/nmodl#1265'.
5bf2d4b
@codecov-commenter

codecov-commenter commented May 15, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 90.62500% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 86.36%. Comparing base (49da686) to head (bb923fd).

Files Patch % Lines
src/utils/blame.cpp 50.00% 2 Missing ⚠️
src/utils/blame.hpp 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1265      +/-   ##
==========================================
+ Coverage   86.35%   86.36%   +0.01%     
==========================================
  Files         175      178       +3     
  Lines       13214    13246      +32     
==========================================
+ Hits        11411    11440      +29     
- Misses       1803     1806       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

bbpbuildbot commented May 15, 2024

Copy link
Copy Markdown
Collaborator

Logfiles from GitLab pipeline #211217 (:white_check_mark:) have been uploaded here!

Status and direct links:

@1uc 1uc marked this pull request as ready for review May 15, 2024 12:59
@JCGoran

JCGoran commented May 16, 2024

Copy link
Copy Markdown
Contributor

Does this mean we'll no longer get multiple stack traces for a single line of NMODL codegen output? If so, I'm very much for it!

@1uc

1uc commented May 21, 2024

Copy link
Copy Markdown
Collaborator Author

Naturally, there are multiple stacks per line of output. Simply because multiple distinct statements can contribute to a single line of code.

What this does is only print one "trace" or frame per stack trace, i.e. per call to the printer that causes changed to specified line. The trace shown will be the lowest call before entering the "printer". The hope is that this is usually the trace of interest. You can see the complete output above (which used to be multiple pages of scrollback).

If in future we notice that sometimes we need more context, e.g. when a generic function calls the printer, we can implement a flag blame --detailed --line N and print the old complete backtrace for each call to the printer. In anticipation, it introduces an interface class called Blame. When the need arises we'll stop passing through the blame_line and instead pass in a (smart pointer to a) Blame object, and create a new subclass DetailedBlame with the desired behaviour.

JCGoran
JCGoran approved these changes May 22, 2024
View reviewed changes

@JCGoran JCGoran left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just checked, a multi-thousand line backtrace was reduced to something like 30 lines, much nicer to debug things now! LGTM

@1uc

1uc commented May 22, 2024

Copy link
Copy Markdown
Collaborator Author

I'm wondering what miserable line you're debugging =)

@1uc 1uc merged commit 24706b5 into master May 22, 2024
@1uc 1uc deleted the 1uc/improve-blame branch May 22, 2024 12:24
@JCGoran JCGoran mentioned this pull request May 27, 2024
Closed
JCGoran pushed a commit to neuronsimulator/nrn that referenced this pull request Mar 12, 2025
@1uc
Refactor nmodl ... blame --line N. (BlueBrain/nmodl#1265)
d63b17c 
The revised format only prints the first trace, before entering the
`CodePrinter`.

It also refactors the code to prepare for injecting a detailed blame
printer, when needed.

NMODL Repo SHA: BlueBrain/nmodl@24706b5
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@JCGoran JCGoran JCGoran approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@1uc @codecov-commenter @bbpbuildbot @JCGoran