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

New rule that ensures module/interface/package/program identifier matches the filename it's in #283

Merged
merged 14 commits into from
Jun 3, 2024

Conversation

5han7anu-S
Copy link
Contributor

Closes #252

The pass testcase is slightly unintuitive in that the module name does not actually match the file it's currently in, but works due to how build.rs modifies the filenames for testing.

@DaveMcEwan
Copy link
Contributor

Nice one @ShantanuPSinha :)
The . was used in the testcase filenames specifically because it's a character that can't appear in an identifier!

To get the testcases working, I'd like to suggest a different approach (not replacing . with _): Just match everything in the filename up to the first non-identifier character.
As well as Foo.1of5.sv (like the testcases), that would allow these filenames to contain a module Foo: Foo.v, Foo.bugfix.sv, Foo-whatever.weird, etc. for all those places that have weird and wonderful tool flows.

@5han7anu-S
Copy link
Contributor Author

5han7anu-S commented Apr 2, 2024

Thanks @DaveMcEwan!

Just a couple of clarifying questions.

  1. module foo; is allowed to live in any file of format Foo<Non-Identifier><whatever else> and the stopping point for string matching has to be a non-identifier character? This way FooBar.sv is an invalid location, but Foo-Bar.sv is a valid filename for module Foo;.

  2. For cargo test, after restoring the periods in test file names in build.rs, the filename now looks like syntaxrules.module_identifier_matches_filename.pass.1of1 Matching everything in the filename up to the first non-identifier character, would now only capture syntaxrules. The testcases, again, can be modified such that the pass testcase is module syntaxrules;, but this may make for unintuitive reading of the manual. Do you have any thoughts or ideas on this?

  3. Should I enforce case-sensitivity (i.e. module foo; has to be in foo.sv and Foo.sv will fail) or is that up to TextRules?

Thanks for your time

@5han7anu-S
Copy link
Contributor Author

Updates:

  • Made documentation explanations clearer.
  • File Identifier is now a substring from the filename up to the first non-identifier. This file identifier is then compared to the module/package/interface/program identifier.
  • Testcases are updated with an explanation for why module syntaxrules; passes the testcase. I believe this explanation resolves the issue of the manual feeling unintuitive.

Notes: I am using str::eq_ignore_ascii_case to compare the file identifier with the test identifier. This is case insensitive. Changing it to account for case sensitivity is trivial, however, does Case matter in the context of this rule? I do not see a scenario where a user has module foo; and module Foo; in the same scope and intends for them to be two distinct modules.

@5han7anu-S
Copy link
Contributor Author

Hey @DaveMcEwan, do you have any feedback/suggestions?

@5han7anu-S
Copy link
Contributor Author

@DaveMcEwan @dalance, just following up on this

@dalance
Copy link
Owner

dalance commented Jun 3, 2024

I think this PR seems to be ready, so I'll merge it.

@dalance dalance merged commit 65fffe0 into dalance:master Jun 3, 2024
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.

New Rule: Check that filename matches module/interface/package/program identifier.
3 participants