First proposal of improvements on C++ generator #1065

tristan0x · 2023-09-06T08:05:24Z

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 · 2023-09-06T08:13:05Z

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 · 2023-09-06T14:08:01Z

@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 · 2023-09-06T14:33:28Z

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

ohm314

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

src/printer/code_printer.hpp

src/codegen/codegen_acc_visitor.cpp

src/codegen/codegen_cpp_visitor.cpp

codecov · 2023-09-18T15:12:30Z

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.

* 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

src/codegen/codegen_acc_visitor.cpp

src/printer/code_printer.cpp

Co-authored-by: Nicolas Cornu <[email protected]>

iomaganaris

LGTM overall. Some small suggestions

src/printer/code_printer.cpp

src/codegen/codegen_cpp_visitor.cpp

Co-authored-by: Ioannis Magkanaris <[email protected]>

tristan0x · 2023-09-21T11:06:06Z

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

…inter

bbpbuildbot · 2023-09-21T13:04:46Z

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

Status and direct links:

pramodk · 2023-09-21T14:23:39Z

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

* 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 <[email protected]> Co-authored-by: Ioannis Magkanaris <[email protected]> Co-authored-by: Erik Heeren <[email protected]>

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

ohm314 reviewed Sep 6, 2023

View reviewed changes

tristan0x force-pushed the tristan0x/enh/code-printer branch 3 times, most recently from 87e724f to 453cc1a Compare September 8, 2023 16:20

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

tristan0x force-pushed the tristan0x/enh/code-printer branch from 5eb5a81 to 0e59bd4 Compare September 19, 2023 09:10

This comment has been minimized.

Sign in to view

tristan0x marked this pull request as ready for review September 19, 2023 10:09

tristan0x requested a review from iomaganaris September 19, 2023 10:09

alkino requested changes Sep 19, 2023

View reviewed changes

src/codegen/codegen_acc_visitor.cpp Outdated Show resolved Hide resolved

src/codegen/codegen_acc_visitor.cpp Outdated Show resolved Hide resolved

src/printer/code_printer.cpp Outdated Show resolved Hide resolved

tristan0x and others added 5 commits September 19, 2023 13:29

Update src/codegen/codegen_acc_visitor.cpp

84541eb

Co-authored-by: Nicolas Cornu <[email protected]>

take @alkino comment into account

6775a0a

CodePrinter::pop_block: remove param / always add NL

d9bd59e

CodePrinter::pop_block_nl: improve prototype

f948f67

Fix previous patch

2300a54

This comment has been minimized.

Sign in to view

iomaganaris reviewed Sep 19, 2023

View reviewed changes

src/printer/code_printer.cpp Show resolved Hide resolved

src/printer/code_printer.cpp Outdated Show resolved Hide resolved

src/codegen/codegen_cpp_visitor.cpp Outdated Show resolved Hide resolved

Update src/printer/code_printer.cpp

4c9b8a1

Co-authored-by: Ioannis Magkanaris <[email protected]>

take @iomaganaris comment into account

6e879a2

This comment has been minimized.

Sign in to view

alkino approved these changes Sep 20, 2023

View reviewed changes

iomaganaris approved these changes Sep 20, 2023

View reviewed changes

tristan0x and others added 2 commits September 21, 2023 11:35

CodegenCppVisitor::visit_program more visible

e2c30b9

Try removing python3-importlib-metadata

2b7de5f

This comment has been minimized.

Sign in to view

Merge branch 'heerener/importlib_metadata' into tristan0x/enh/code-pr…

c767d70

…inter

tristan0x merged commit 4a6d511 into master Sep 21, 2023
16 checks passed

tristan0x deleted the tristan0x/enh/code-printer branch September 21, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First proposal of improvements on C++ generator #1065

First proposal of improvements on C++ generator #1065

tristan0x commented Sep 6, 2023 •

edited

Loading

tristan0x commented Sep 6, 2023

pramodk commented Sep 6, 2023

ohm314 commented Sep 6, 2023

ohm314 left a comment

This comment has been minimized.

codecov bot commented Sep 18, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

iomaganaris left a comment

This comment has been minimized.

This comment has been minimized.

tristan0x commented Sep 21, 2023

bbpbuildbot commented Sep 21, 2023

pramodk commented Sep 21, 2023

First proposal of improvements on C++ generator #1065

First proposal of improvements on C++ generator #1065

Conversation

tristan0x commented Sep 6, 2023 • edited Loading

tristan0x commented Sep 6, 2023

Stack approach to manipulate code blocks

Rework CodePrinter::add_line

Remove trivial fmt::format

pramodk commented Sep 6, 2023

ohm314 commented Sep 6, 2023

ohm314 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

iomaganaris left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

tristan0x commented Sep 21, 2023

bbpbuildbot commented Sep 21, 2023

pramodk commented Sep 21, 2023

tristan0x commented Sep 6, 2023 •

edited

Loading

Remove trivial `fmt::format`

codecov bot commented Sep 18, 2023 •

edited

Loading