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

First proposal of improvements on C++ generator#1065

Merged
tristan0x merged 11 commits into
masterfrom
tristan0x/enh/code-printer
Sep 21, 2023
Merged

First proposal of improvements on C++ generator#1065
tristan0x merged 11 commits into
masterfrom
tristan0x/enh/code-printer

Conversation

@tristan0x

@tristan0x tristan0x commented Sep 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member
  • Reduce number of copies
  • CodePrinter::add_text:
    • now supports multiple arguments
    • use it instead of trivial but expensive calls to fmt::format.
  • CodePrinter::add_multi_line:
    • Rework the implementation to support C++ raw string literals
    • Use it instead of 3 or more consecutive calls to add_line
  • CodePrinter::start_block: rename to push_block
  • CodePrinter::end_block: rename to pop_block
  • CodePrinter::chain_block: rename to chain_block

@tristan0x tristan0x requested review from ohm314 and pramodk September 6, 2023 08:05
@tristan0x

tristan0x commented Sep 6, 2023

Copy link
Copy Markdown
Member Author

Initial discussion : @pramodk @ohm314

Stack approach to manipulate code blocks

CodePrinter::start_block: rename to push_block
CodePrinter::end_block: rename to pop_block

Rework CodePrinter::add_line

The change allows the method to essentially work like the Python standard library function textwrap.dedent

Remove trivial fmt::format

It costs ~8x more to do CodePrinter::fmt_line("{} bar", var_name) than CodePrinter::add_line(var_name, " bar") (I did a micro-benchmark). I would like to replace these calls and emphase in the documentation that fmt should be used only when the formatting is complex or add_line(...) would make the code hard to read.

@pramodk

pramodk commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

@tristan0x: suggestions look fine to me! (as mentioned elsewhere, we would like to make it easy to understand/develop/contribute by external users/devs)

@ohm314

ohm314 commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

(Bear with me until I look in detail at the changes but) overall I agree with the suggestions.

ohm314
ohm314 reviewed Sep 6, 2023
View reviewed changes

@ohm314 ohm314 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.

Alright, I did a pass. I really like the changes. Had just some minor comments.

Comment thread src/printer/code_printer.hpp Outdated
Comment thread src/printer/code_printer.hpp Outdated
Comment thread src/codegen/codegen_acc_visitor.cpp Outdated
Comment thread src/codegen/codegen_cpp_visitor.cpp Outdated
Comment thread src/codegen/codegen_cpp_visitor.cpp
Comment thread src/codegen/codegen_cpp_visitor.cpp
@tristan0x tristan0x force-pushed the tristan0x/enh/code-printer branch 3 times, most recently from 87e724f to 453cc1a Compare September 8, 2023 16:20
@bbpbuildbot

This comment has been minimized.

Sign in to view
@tristan0x tristan0x force-pushed the tristan0x/enh/code-printer branch from c4e7fce to 97dab1e Compare September 18, 2023 14:25
@codecov

codecov Bot commented Sep 18, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 90.59% and project coverage change: +0.24% 🎉

Comparison is base (441a3b1) 69.94% compared to head (c767d70) 70.19%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1065      +/-   ##
==========================================
+ Coverage   69.94%   70.19%   +0.24%     
==========================================
  Files         188      188              
  Lines       25647    25632      -15     
==========================================
+ Hits        17939    17992      +53     
+ Misses       7708     7640      -68
Files Changed Coverage Δ
src/codegen/codegen_acc_visitor.hpp 50.00% <ø> (ø)
src/symtab/symbol_properties.cpp 58.69% <0.00%> (+11.88%) ⬆️
src/utils/string_utils.hpp 100.00% <ø> (+5.71%) ⬆️
src/codegen/codegen_info.cpp 70.73% <75.00%> (ø)
src/symtab/symbol_table.hpp 76.66% <77.77%> (+1.66%) ⬆️
src/codegen/codegen_acc_visitor.cpp 64.10% <85.00%> (-0.26%) ⬇️
src/codegen/codegen_cpp_visitor.cpp 85.56% <89.65%> (-0.16%) ⬇️
src/printer/code_printer.cpp 94.20% <96.96%> (+7.76%) ⬆️
src/codegen/codegen_compatibility_visitor.cpp 100.00% <100.00%> (ø)
src/codegen/codegen_compatibility_visitor.hpp 100.00% <100.00%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

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

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@tristan0x tristan0x force-pushed the tristan0x/enh/code-printer branch from 6adfd7b to 5eb5a81 Compare September 19, 2023 07:43
@bbpbuildbot

This comment has been minimized.

Sign in to view
@tristan0x
First proposal of improvements on C++ generator
0e59bd4 
* reduce number of copies
* `CodePrinter::add_text`:
  * now supports multiple arguments
  * use it instead of trivial but expensive call to `fmt::format`.
* `CodePrinter::add_multi_line`:
  * rework to work with C++ raw string literals
  * used it instead of 3 or more consecutive `add_line`
* `CodePrinter::start_block`: rename to `push_block`
* `CodePrinter::end_block`: rename to pop_block
@tristan0x tristan0x force-pushed the tristan0x/enh/code-printer branch from 5eb5a81 to 0e59bd4 Compare September 19, 2023 09:10
@bbpbuildbot

This comment has been minimized.

Sign in to view
@tristan0x tristan0x marked this pull request as ready for review September 19, 2023 10:09
alkino
alkino suggested changes Sep 19, 2023
View reviewed changes
Comment thread src/codegen/codegen_acc_visitor.cpp Outdated
Comment thread src/codegen/codegen_acc_visitor.cpp Outdated
Comment thread src/printer/code_printer.cpp Outdated
@bbpbuildbot

This comment has been minimized.

Sign in to view
iomaganaris
iomaganaris reviewed Sep 19, 2023
View reviewed changes

@iomaganaris iomaganaris 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.

LGTM overall. Some small suggestions

Comment thread src/printer/code_printer.cpp
Comment thread src/printer/code_printer.cpp
Comment thread src/codegen/codegen_cpp_visitor.cpp Outdated
@tristan0x @iomaganaris
Update src/printer/code_printer.cpp
4c9b8a1 
Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com>
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@tristan0x

tristan0x commented Sep 21, 2023

Copy link
Copy Markdown
Member Author

The CI has failed since yesterday with a weird setuptools_scm issue. Any idea where I can loop @pramodk @ohm314 @heerener ?

@bbpbuildbot

bbpbuildbot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@tristan0x tristan0x merged commit 4a6d511 into master Sep 21, 2023
@tristan0x tristan0x deleted the tristan0x/enh/code-printer branch September 21, 2023 13:51
@pramodk

pramodk commented Sep 21, 2023

Copy link
Copy Markdown
Contributor

Sorry, I was too late to notice comment. But I guess this has resolved somehow 👍

ohm314 pushed a commit that referenced this pull request Oct 5, 2023
@tristan0x @alkino
@iomaganaris @heerener
First proposal of improvements on C++ generator (#1065)
583b096 
* First proposal of improvements on C++ generator

* reduce number of copies
* `CodePrinter::add_text`:
  * now supports multiple arguments
  * use it instead of trivial but expensive call to `fmt::format`.
* `CodePrinter::add_multi_line`:
  * rework to work with C++ raw string literals
  * used it instead of 3 or more consecutive `add_line`
* `CodePrinter::start_block`: rename to `push_block`
* `CodePrinter::end_block`: rename to pop_block
* CodegenCppVisitor::visit_program more visible
* Try removing python3-importlib-metadata

Co-authored-by: Nicolas Cornu <nicolas.cornu@epfl.ch>
Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com>
Co-authored-by: Erik Heeren <erik.heeren@epfl.ch>
JCGoran pushed a commit to neuronsimulator/nrn that referenced this pull request Mar 12, 2025
@tristan0x
First proposal of improvements on C++ generator (BlueBrain/nmodl#1065)
2adf046 
* First proposal of improvements on C++ generator

* reduce number of copies
* `CodePrinter::add_text`:
  * now supports multiple arguments
  * use it instead of trivial but expensive call to `fmt::format`.
* `CodePrinter::add_multi_line`:
  * rework to work with C++ raw string literals
  * used it instead of 3 or more consecutive `add_line`
* `CodePrinter::start_block`: rename to `push_block`
* `CodePrinter::end_block`: rename to pop_block
* CodegenCppVisitor::visit_program more visible
* Try removing python3-importlib-metadata

Co-authored-by: Nicolas Cornu <nicolas.cornu@epfl.ch>
Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com>
Co-authored-by: Erik Heeren <erik.heeren@epfl.ch>

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

Reviewers

@pramodk pramodk Awaiting requested review from pramodk
3 more reviewers
@ohm314 ohm314 ohm314 left review comments
@alkino alkino alkino approved these changes
@iomaganaris iomaganaris iomaganaris 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.

7 participants

@tristan0x @pramodk @ohm314 @bbpbuildbot @alkino @iomaganaris @heerener