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

Add lexer detection of translate_on/off pragmas #1157

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

povik
Copy link
Contributor

@povik povik commented Oct 17, 2024

Follow-up to #1100 with a changed approach. The detection now happens in lexer, not preprocessor. Tests are missing.

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.63%. Comparing base (f31d20e) to head (7e2329d).
Report is 31 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
- Coverage   94.84%   94.63%   -0.21%     
==========================================
  Files         191      195       +4     
  Lines       47944    50261    +2317     
==========================================
+ Hits        45472    47566    +2094     
- Misses       2472     2695     +223     
Files with missing lines Coverage Δ
include/slang/parsing/Lexer.h 100.00% <ø> (ø)
source/parsing/Lexer.cpp 99.51% <100.00%> (+0.03%) ⬆️

... and 159 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

Copy link
Owner

@MikePopoloski MikePopoloski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach seems ok. I think there must be a way to optimize it a bit more to avoid going into the full loop to check for cases since 99.9% of comments won't match, but it's probably ok for now.

You do need to add tests though; ideally you get full coverage of all branches, especially for something critical like the lexer.

source/parsing/Lexer.cpp Outdated Show resolved Hide resolved
@povik
Copy link
Contributor Author

povik commented Nov 14, 2024

You do need to add tests though; ideally you get full coverage of all branches, especially for something critical like the lexer.

OK, I have started some testing, let me see what the report will say

@povik
Copy link
Contributor Author

povik commented Nov 14, 2024

Looks like the coverage is full

@MikePopoloski MikePopoloski merged commit 862b51c into MikePopoloski:master Nov 14, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants