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

Suggestion: Move parameter into module in number_test #1303

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Keno
Copy link

@Keno Keno commented Jan 28, 2021

I'm working on validating a parser for Verilog-AMS, which is
basically vanilla verilog + extensions + some backports from
SystemVerilog. Because of this shared heritage, quite a number
of the tests in this repository are applicable and helpful.
The number_test tests are useful also, because they test
lexing of numeric literals. There are two tests (_3 and _4)
that test SystemVerilog extensions, but the remainder are
Vanilla verilog modulo the fact that parameter statements
can't appear at top level in vanilla Verilog. By moving
the parameter statements inside the model, the test would
be valid in both vanilla and System Verilog. I understand
that this is not a Verilog-AMS test suite, but I was hoping
you might be open to generalizing the test files where the
particular feature being tested itself is not a SystemVerilog
extension, such that we may share some of these testcases
between this repository and our Verilog-AMS test suite.

I'm working on validating a parser for Verilog-AMS, which is
basically vanilla verilog + extensions + some backports from
SystemVerilog. Because of this shared heritage, quite a number
of the tests in this repository are applicable and helpful.
The number_test tests are useful also, because they test
lexing of numberic literals. There are two tests (_3 and _4)
that test SystemVerilog extensions, but the remainder are
Vanilla verilog modulo the fact that `parameter` statements
can't appear at top level in vanilla Verilog. By moving
the parameter statements inside the model, the test would
be valid in both vanilla and System Verilog. I understand
that this is not a Verilog-AMS test suite, but I was hoping
you might be open to generalizing the test files where the
particular feature being tested itself is not a SystemVerilog
extension, such that we may share some of these testcases
between this repository and our Verilog-AMS test suite.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
@caryr
Copy link
Contributor

caryr commented Jan 29, 2021

I personally have no objection, but we do want to have some tests that exercise the SystemVerilog functionality so I do not agree with just switching all these to be declared inside the module.

I'll also comment ivtest has some Verilog-A tests, but they are obviously not loaded by sv-tests.

@tgorochowik tgorochowik requested a review from hzeller February 5, 2021 16:30
@hzeller
Copy link
Collaborator

hzeller commented Feb 5, 2021

Given that these tests strictly just test for parseability of number literals, I am not opposed to making these also digestable by pure Verilog parsers.

One request though: please make sure to indent the parameter line within the module block. Not sure if there is even a super-consistent style (2 or 4 spaces) in the test-suite, but I suggest 2.

module test;
  parameter test = 0;
endmodule

@caryr which is the test you'd be worried about ? Or in general you suggest to have at least one test that has a parameter outside the module body ? For that, I'd probably suggest that we add a test in tests/chapter-6/6.20.2--... somewhere.

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, but please add the proper indentation.

Once we know how to proceed with 'parameter outside module' question, I might also ask you to add one test of a parameter outside a module in the chapter-6 area to cover the accidental covering of that feature.

@caryr
Copy link
Contributor

caryr commented Feb 6, 2021

My request is to have at least one with the parameter outside the module definition to check that parameters can be declared outside a module. To be thorough we would want to cover every combination both inside and outside the module, but that is likely being overly cautious. I'm fine with moving all these inside for this section and creating a copy to check that out of module parameter declarations work correctly in a different section.

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.

3 participants