Skip to content

Change inflection on unions #212

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

Merged
merged 1 commit into from
May 17, 2018

Conversation

Emilgardis
Copy link
Member

PascalCase doesn't work when we have overloaded registers named
FOO_11 and FOO1_1.

This happens in

  • Freescale MKL81Z7
  • Freescale MKL82Z7
  • Freescale MK81F25615
  • Freescale MK82F25615
    on LTC0_PK* registers.

I'm not sure if {ident}Union is the best choice anymore as FOO_BARUnion doesn't seem as good as it possibly could be.

cc @jamesmunns @wez

@wez
Copy link
Contributor

wez commented May 9, 2018

Bummer. I'm not super opinionated on the naming here. Should we embrace uppercase and go for FOO_BAR_UNION?

@Emilgardis Emilgardis force-pushed the fix-regress-pascal-case branch from d89e1f4 to d4fcccd Compare May 9, 2018 09:34
@Emilgardis Emilgardis requested a review from jamesmunns May 9, 2018 09:38
@Emilgardis
Copy link
Member Author

I switched it over to {}_UNION, @jamesmunns you talked briefly about the naming in #192
I ran this against svd2rust-regress and it's all clear, however I'm not sure if this naming is ideal

@jamesmunns
Copy link
Member

jamesmunns commented May 9, 2018

Hey, ACKing that I've seen this, I don't have strong opinions on naming (other than it should be consistent).

I think I need some time to grok what is going on here, and see if I can think of anything clever. I probably will be able to get to this by Monday (feel free to re-ping me if I have forgotten :) )

@Emilgardis
Copy link
Member Author

ping @jamesmunns

@jamesmunns
Copy link
Member

jamesmunns commented May 15, 2018

Argh. I knew there was something I was forgetting. Thanks @Emilgardis.

I'm not crazy about naming the unions as upper case, as that is typically used for constants, and is a bit of a wierd interface for users. I'd rather have the naming "weird" for these edge cases, and less weird for normal usage, rather than making it equally weird for everyone.

I don't want to hold up something that works, because I don't have particularly strong feelings about naming either.

I guess my one bike shed thought would be to introduce something like Pascal_Snake_Case for unions, which would look like this:

<name>LTC0_PKA1_1</name> => Ltc0_Pka1_1_Union
<name>LTC0_PKA_11</name> => Ltc0_Pka_11_Union

Or replacing _ with a word, such as Sub:

<name>LTC0_PKA1_1</name> => Ltc0SubPka1Sub1SubUnion
<name>LTC0_PKA_11</name> => Ltc0SubPkaSub11SubUnion

But honestly this is even bad, as it introduces a new, weird naming convention, or inserts a lot of noise into words.

If no one really cares, lets go with UPPER_CASE for now, and have a follow up discussion before we move union support into the stable part of svd2rust

edit: add the _Union suffix to the examples

@Emilgardis
Copy link
Member Author

@jamesmunns re unions, we may want to move away from them, see #218

I'll wait a bit and then r+, let's see if anyone has a better solution

Copy link
Member Author

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

#L10, unused import ToSanitizedPascalCase

PascalCase doesn't work when we have registers named
`FOO_11` and `FOO1_1`.

This happened in
* Freescale MKL81Z7
* Freescale MKL82Z7
* Freescale MK81F25615
* Freescale MK82F25615
on `LTC0_PK*` registers.
@Emilgardis Emilgardis force-pushed the fix-regress-pascal-case branch from d4fcccd to 6e6b149 Compare May 15, 2018 22:52
@Emilgardis
Copy link
Member Author

passed locally with svd2rust-regress, merging manually

@Emilgardis Emilgardis merged commit 8a50bc1 into rust-embedded:master May 17, 2018
@Emilgardis Emilgardis deleted the fix-regress-pascal-case branch May 17, 2018 12:02
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.

3 participants