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.

Sign up for GitHub

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

Jump to bottom

seperator of signal names in records causes ambiguity #48

Closed
anuejn opened this issue Mar 24, 2019 · 5 comments · Fixed by #50
Closed

seperator of signal names in records causes ambiguity #48

anuejn opened this issue Mar 24, 2019 · 5 comments · Fixed by #50
Labels
improvement
Milestone
0.1

Comments

@anuejn
Copy link
Contributor

anuejn commented Mar 24, 2019

When creating a Record, the signals inside it are named recordname_fieldname currently.

This makes it hard to see the hierarchy when using snake case names for records and fields as well. I.e. having a record named some_record with a field named some_field the resulting signal is called some_record_some_field which could also be the name of a signal in a nested record with depth 3.

I propose to use another character for separation (for example .), which would lead to some_record.some_field in that case.

(see: https://github.com/m-labs/nmigen/blob/master/nmigen/hdl/rec.py#L75)
@whitequark
Copy link
Contributor

This ambiguity was something I expected when writing that code. The problem with . is that it's not a valid Verilog identifier character and so that will result in all Verilog identifiers with the dot being escaped. IMO that's about as annoying and also hard to read. But maybe we can still change that.

@anuejn
Copy link
Contributor Author

anuejn commented Mar 25, 2019

What about using the $ sign instead? That wouldnt need to be escaped but collides with other usages of that character (however not as bad as _ imho). Alternatively, one could use a multi character seperator like _$ or __.

what do you think?

@whitequark
Copy link
Contributor

I think __ makes the most sense out of these options.

@anuejn
Copy link
Contributor Author

anuejn commented Mar 25, 2019

I like that :). Should I create a PR?

@whitequark
Copy link
Contributor

Sure.

@anuejn anuejn mentioned this issue Mar 25, 2019
Merged
@whitequark whitequark added this to the 0.1 milestone Jun 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
0.1
Development

Successfully merging a pull request may close this issue.

Change name seperator for siganls in records to double underscore
2 participants
@whitequark @anuejn