search

PLC-lang / rusty

Structured Text Parser and LLVM Frontend
GNU Lesser General Public License v3.0
183 stars 48 forks source link

String rcs #364

Closed ghaith closed 2 years ago

ghaith commented 2 years ago

Experiment: Move Strings in AST to RC

ghaith commented 2 years ago

@riederm what do you think about this? There isn't much overhead with adding RCs, just a lot of places to change. I'm not sure how much more efficient this is than just cloning strings in our use-case though.

codecov-commenter commented 2 years ago

Codecov Report

Merging #364 (60d430d) into master (da0d1dc) will increase coverage by 0.00%. The diff coverage is 97.77%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #364   +/-   ##
=======================================
  Coverage   93.48%   93.48%           
=======================================
  Files          30       30           
  Lines        9558     9561    +3     
=======================================
+ Hits         8935     8938    +3     
  Misses        623      623
Impacted Files Coverage Δ
src/typesystem.rs 95.91% <ø> (ø)
src/index/visitor.rs 95.82% <75.00%> (ø)
src/ast.rs 93.28% <100.00%> (ø)
src/ast/pre_processor.rs 100.00% <100.00%> (ø)
src/index/const_expressions.rs 93.38% <100.00%> (ø)
src/parser.rs 98.03% <100.00%> (ø)
src/test_utils.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update da0d1dc...60d430d. Read the comment docs.

riederm commented 2 years ago

@riederm what do you think about this? There isn't much overhead with adding RCs, just a lot of places to change. I'm not sure how much more efficient this is than just cloning strings in our use-case though.

I quickly scanned over it, and I was a bit surprised how little effect this has ... the main effect I see so far, is in the preprocessor, but only for the scope's of datatypes. I'm also not sure if and how it will help when working with qualified names and constructing qualifiers, or fully qualified names on the go :-/

The effect on the AST is maybe not so big :-/ Lets discuss this further.

ghaith commented 2 years ago

Keep in mind that i also avoided the data types and index so far. I think this would have a bigger impact when I reach it. But again not sure if it's worth it. We don't have huge amount of strings we are cloning. So optimization there might be wasted effort for now

On Sun, 7 Nov 2021, 20:14 Mathias Rieder, @.***> wrote:

@riederm https://github.com/riederm what do you think about this? There isn't much overhead with adding RCs, just a lot of places to change. I'm not sure how much more efficient this is than just cloning strings in our use-case though.

I quickly scanned over it, and I was a bit surprised how little effect this has ... the main effect I see so far, is in the preprocessor, but only for the scope's of datatypes. I'm also not sure if and how it will help when working with qualified names and constructing qualifiers, or fully qualified names on the go :-/

The effect on the AST is maybe not so big :-/ Lets discuss this further.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/PLC-lang/rusty/pull/364#issuecomment-962665102, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIRVS6AMV2CC6WSXQV5ULUK3FXTANCNFSM5HP6ZKYA .

riederm commented 2 years ago

Keep in mind that i also avoided the data types and index so far.

Yes ... for my feeling the index duplicates the strings, but the whole situation with qualified names (where you want to try sthings like: "variablename", "method.variablename", "pou.method.variablename") is not getting any better whit RCs?