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

Enforce naming rules on core HDL #1235

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tpwrules
Copy link
Contributor

The rules are quite simple and liberal:

  • Can't be None or the empty string (case-specific exceptions)
  • Can't contain a Unicode separator character
  • Can't contain a Unicode control character, undefined character, or surrogate character

I did not bother updating the deprecated _mem or _rec modules.

Pretty much all cases I tried using a name illegal by these rules, it caused a problem somewhere else (e.g. an RTLIL syntax error).

Maybe worth adding some logic to Fragment to catch any cases that slipped through?

Closes #1209

@tpwrules
Copy link
Contributor Author

tpwrules commented Mar 24, 2024

Apparently having arbitrary FSM state is desirable, so those will be allowed again once #1234 is merged. This will need a patch after then anyway.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.82%. Comparing base (fa2adbe) to head (e0bbaca).
Report is 189 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1235      +/-   ##
==========================================
- Coverage   89.87%   89.82%   -0.05%     
==========================================
  Files          43       43              
  Lines        9992    10008      +16     
  Branches     2417     2417              
==========================================
+ Hits         8980     8990      +10     
- Misses        818      822       +4     
- Partials      194      196       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

My overall feelings about this PR:

  • The general approach with the use of unicodedata seems good.
  • I don't like having none_ok and empty_ok. There are already complex conditionals around the use of names in multiple places; adding an invisible at the call site complex conditional to this isn't making it easier to understand.
  • I don't like how the error messages are no longer greppable because they are composed from multiple parts. I've tried to avoid this elsewhere because it makes it harder to figure out exactly where a message comes from if you don't have a backtrace (eg someone stringified an exception, or you just want to find every place where it's mentioned).
  • I don't like that "what" is duplicated several times per entity (e.g. "Clock domain name" is independently used 3 times). I think a private class method that would encapsulate all of the naming logic, including what is currently done by none_ok and empty_ok, would do better than the two boolean flags which despite having 4 states reflect 3 possible desired combinations (I don't think we ever want to accept both None and "" by the time the validation is called).
  • I have some uncertainty about whether attributes and instance named parameters/ports should accept Unicode. Especially attributes.

I think for 0.5 I just want the linked issue fixed as a one-off because this PR has too many moving parts that I'm uncertain about.

@whitequark whitequark modified the milestones: 0.5, 0.6 Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Zero-length submodule name breaks things
2 participants