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

Bug fixes for empty port list in module/interface and triple quoted string support #5

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

yuhaos
Copy link
Contributor

@yuhaos yuhaos commented Sep 23, 2024

Hi Gmlarumbe,

This patch is for the following two issues, may you please have a review? I upload grammar.js only.

  • fix the issues the empty port list is not regonized
  • the original implementation could not detected triple quoted string correctly, fix it.

Regards
Yuhao

The test code is quite simple:

module top();

initial begin
  string a = """
  a, b, c, "xy"
  """;
  $display("good");
end

endmodule

@gmlarumbe
Copy link
Owner

Hi @yuhaos ,

Thanks a lot for your PR.

A few comments:

  • I might be missing something but the empty port list should be supported without the PR changes.

    • Check the following test files that are passing CI:
      • test/files/core/module/empty_1.sv
      • test/files/core/module/param_empty.sv
    • The optional wrapping commaSep should not be needed either since commaSep matches 0 or more comma separated values. commaSep calls sepBy which in turn includes optional already.
    • This piece of code gives me no errors:
      module top();
      endmodule
  • The changes to support the triple_quoted_string look good to me. Thanks a lot for taking the time to debug these.

  • Regarding the renaming from verilog to systemverilog that is something that probably could be done in a separate PR to account for the generated files.

Could you please check these points and rebase/force_push before merging? Could you also include only the grammar.js changes for triple_quoted_string and triple_quoted_string_itemin the PR?

Thanks!

triple string might have " or "" inside, update the definition of triple
string to support such scenario.
@yuhaos
Copy link
Contributor Author

yuhaos commented Sep 28, 2024

Hi @gmlarumbe ,

Thanks for the explanation, the empty port list is supported in the original implementation, my change is redundant.
I re-submit grammar.js, may you have a review? Thanks.

Regards
Yuhao

@yuhaos yuhaos reopened this Sep 28, 2024
@gmlarumbe gmlarumbe merged commit a092d48 into gmlarumbe:master Sep 28, 2024
4 checks passed
@gmlarumbe
Copy link
Owner

Thanks a lot for your contribution! :)

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