Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(semantic): add cfg nodes for ConditionalExpressions. #3127

Merged

Conversation

rzvxa
Copy link
Collaborator

@rzvxa rzvxa commented Apr 29, 2024

It is done similarly to how IfStatements are structured at the moment.

@github-actions github-actions bot added the A-semantic Area - Semantic label Apr 29, 2024
Copy link

codspeed-hq bot commented Apr 29, 2024

CodSpeed Performance Report

Merging #3127 will not alter performance

Comparing 04-29-fix_semantic_add_cfg_nodes_for_conditionalexpression_s (541a9b9) with main (c91d261)

Summary

✅ 27 untouched benchmarks

@rzvxa rzvxa force-pushed the 04-28-fix_connect_test_expression_of_for_statements_to_the_cfg branch from d24c226 to 2a8148a Compare May 9, 2024 12:14
@rzvxa rzvxa force-pushed the 04-29-fix_semantic_add_cfg_nodes_for_conditionalexpression_s branch from 84f7348 to b76f2e0 Compare May 9, 2024 12:14
@rzvxa
Copy link
Collaborator Author

rzvxa commented May 10, 2024

CodSpeed Performance Report

Merging #3127 will improve performances by ×2.7

Comparing 04-29-fix_semantic_add_cfg_nodes_for_conditionalexpression_s (b76f2e0) with 04-28-fix_connect_test_expression_of_for_statements_to_the_cfg (d24c226)

Summary

⚡ 16 improvements ✅ 11 untouched benchmarks

⁉️ 3 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark 04-28-fix_connect_test_expression_of_for_statements_to_the_cfg 04-29-fix_semantic_add_cfg_nodes_for_conditionalexpression_s Change
lexer[antd.js] 129.9 ms 117 ms +10.99%
lexer[checker.ts] 73 ms 66.5 ms +9.8%
lexer[pdf.mjs] 19.4 ms 18.7 ms +4.17%
⁉️ linter[RadixUIAdoptionSection.jsx] 7.3 ms N/A N/A
⁉️ linter[antd.js] 10.3 s N/A N/A
⁉️ linter[pdf.mjs] 1.8 s N/A N/A
minifier[react.development.js] 9.1 ms 8.7 ms +5.45%
minifier[typescript.js] 1.4 s 1.3 s +7.31%
parser[RadixUIAdoptionSection.jsx] 292.4 µs 281.9 µs +3.75%
parser[antd.js] 599.8 ms 565.9 ms +5.98%
parser[cal.com.tsx] 129.6 ms 124.3 ms +4.27%
parser[checker.ts] 306.1 ms 288.6 ms +6.08%
parser[pdf.mjs] 96.5 ms 92.1 ms +4.76%
semantic[pdf.mjs] 145.7 ms 140.1 ms +4.01%
transformer[RadixUIAdoptionSection.jsx] 919.8 µs 536.8 µs +71.36%
transformer[antd.js] 1,496.2 ms 673.8 ms ×2.2
transformer[cal.com.tsx] 378.5 ms 164.3 ms ×2.3
transformer[checker.ts] 940 ms 352.3 ms ×2.7
transformer[pdf.mjs] 263.7 ms 108.6 ms ×2.4

What is wrong with this benchmark?

@rzvxa rzvxa marked this pull request as ready for review May 10, 2024 13:12
@rzvxa
Copy link
Collaborator Author

rzvxa commented May 10, 2024

Any chance that benchmark is wrong? It was perfectly fine a few days ago and now it shows regression after updating it to the new head.

Copy link
Member

Boshen commented May 10, 2024

Merge activity

  • May 10, 10:11 AM EDT: @Boshen started a stack merge that includes this pull request via Graphite.
  • May 10, 10:12 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 10, 10:16 AM EDT: @Boshen merged this pull request with Graphite.

Base automatically changed from 04-28-fix_connect_test_expression_of_for_statements_to_the_cfg to main May 10, 2024 14:11
@Boshen Boshen force-pushed the 04-29-fix_semantic_add_cfg_nodes_for_conditionalexpression_s branch from 1e0b8bc to 541a9b9 Compare May 10, 2024 14:11
@Boshen Boshen merged commit 5e36e0d into main May 10, 2024
26 checks passed
@Boshen Boshen deleted the 04-29-fix_semantic_add_cfg_nodes_for_conditionalexpression_s branch May 10, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants