-
Notifications
You must be signed in to change notification settings - Fork 176
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
base: main
Are you sure you want to change the base?
Conversation
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. |
a1fbbb2
to
c9a6839
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
States are not "names" as such and still allowed to be (relatively) arbitrary objects.
c9a6839
to
e0bbaca
Compare
There was a problem hiding this 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
andempty_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
andempty_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 bothNone
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.
The rules are quite simple and liberal:
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