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.

Sign up for GitHub

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

Jump to bottom

Adding support of +define+ in FileList. #1421

Merged
merged 7 commits into from
Sep 3, 2022
Merged

Adding support of +define+ in FileList. #1421

merged 7 commits into from
Sep 3, 2022

Conversation

karimtera
Copy link
Contributor

This PR implements: ParseSourceFileListFromCommandline(const std::vector<absl::string_view>& cmdline_args)
Which parses the command line arguments into a FileList that now contains a vector holding the definitions passed through +define+.

Note: ParseSourceFileListFromCommandline is not yet used in the preprocessor tool.

hzeller
hzeller requested changes Sep 2, 2022
View reviewed changes
Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

Nice!

verilog/analysis/verilog_project_test.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project_test.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project.h Show resolved Hide resolved
verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project_test.cc Outdated Show resolved Hide resolved
@karimtera
Changes:
57b3b20 
ParseSourceFileListFromCommandline:
- Always uses absl::string_view unless we really need to copy the data.
- Added unit test containing defines, include directories, and source files.
@karimtera
Parse the preprocessor subcommands args into a FileList.
5d18978
@karimtera
'FileList' usage in the preprocessor.
b1dcd93 
'ParseSourceFileListFromCommandline()':
- Fix parsing issue.
- Add better error messages.
- Add more tests.

The preprocessor tool:
- Use 'ParseSourceFileListFromCommandline()' in subcommands.
@karimtera karimtera force-pushed the positional_arguments branch from 7250f04 to b1dcd93 Compare September 2, 2022 23:44
@karimtera
Copy link
Contributor Author

There was an issue reported by @mglb in the original first PR, if the argument is +define+a=b=c, the macro value should be b=c, I added the fix in here, because I was afraid It will be lost in the first PR.

hzeller
hzeller reviewed Sep 3, 2022
View reviewed changes
verilog/analysis/verilog_project_test.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
@hzeller
Copy link
Collaborator

hzeller commented Sep 3, 2022

did you also add a test to confirm that +define+a=b=c works ?

@karimtera
Copy link
Contributor Author

Yup, I added a test for that.

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2022 •
edited
Loading

Codecov Report

Merging #1421 (5ad2767) into master (fae1fd8) will decrease coverage by 0.01%.
The diff coverage is 88.70%.

@@            Coverage Diff             @@
##           master    #1421      +/-   ##
==========================================
- Coverage   92.94%   92.92%   -0.02%     
==========================================
  Files         342      342              
  Lines       24105    24158      +53     
==========================================
+ Hits        22404    22450      +46     
- Misses       1701     1708       +7
Impacted Files Coverage Δ
verilog/tools/preprocessor/verilog_preprocessor.cc 89.65% <84.61%> (-2.27%) ⬇️
verilog/analysis/verilog_project.cc 94.44% <90.62%> (-0.74%) ⬇️
verilog/analysis/verilog_project.h 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hzeller
hzeller approved these changes Sep 3, 2022
View reviewed changes
Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

LGTM.
Do you want to add anything here, or want the TODO 'send defines to preprocessor' in follow-up changes ?

verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved
verilog/tools/preprocessor/verilog_preprocessor.cc Show resolved Hide resolved
@karimtera
Copy link
Contributor Author

LGTM. Do you want to add anything here, or want the TODO 'send defines to preprocessor' in follow-up changes ?

I've already done the 'send defines to preprocessor' in the the remaining PRs.

@hzeller
Copy link
Collaborator

hzeller commented Sep 3, 2022

Alright, so ready to merge ?

@karimtera
Copy link
Contributor Author

Yes, ready to merge.

@hzeller hzeller merged commit 694974a into chipsalliance:master Sep 3, 2022
@hzeller
Copy link
Collaborator

hzeller commented Sep 3, 2022

Alright, it's in!

@karimtera karimtera deleted the positional_arguments branch September 3, 2022 02:38
@karimtera
Copy link
Contributor Author

I will close PR #1373 then.

@hzeller
Copy link
Collaborator

hzeller commented Sep 6, 2022

To link this up: the corresponding issue is #1358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzeller hzeller hzeller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karimtera @hzeller @codecov-commenter