BlueBrain / nmodl

Code Generation Framework For NEURON MODeling Language
https://bluebrain.github.io/nmodl/
Apache License 2.0
48 stars 15 forks source link

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

Closed 1uc closed 1 month ago

1uc commented 1 month ago

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.

codecov-commenter commented 1 month ago

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 :warning:
src/utils/blame.hpp 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bbpbuildbot commented 1 month ago

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

Status and direct links:

JCGoran commented 1 month ago

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 commented 1 month ago

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.

1uc commented 1 month ago

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