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

Add enums for LPC55 pin codegen #1732

Merged
merged 3 commits into from
Apr 15, 2024
Merged

Add enums for LPC55 pin codegen #1732

merged 3 commits into from
Apr 15, 2024

Conversation

mkeeter
Copy link
Collaborator

@mkeeter mkeeter commented Apr 5, 2024

This means that invalid pin configuration parameters will fail to parse, instead of making it through codegen and then failing at (Hubris) compile time.

I don't love leaning on the derived Debug here:

let mode = format_ident!("{}", format!("{:?}", self.get_mode()));
let slew = format_ident!("{}", format!("{:?}", self.get_slew()));
let invert = format_ident!("{}", format!("{:?}", self.get_invert()));
let digimode = format_ident!("{}", format!("{:?}", self.get_digimode()));
let od = format_ident!("{}", format!("{:?}", self.get_opendrain()));
tokens.append_all(final_pin);
tokens.append_all(quote::quote! {
    AltFn::#alt_num,
    Mode::#mode,
    Slew::#slew,
    Invert::#invert,
    Digimode::#digimode,
    Opendrain::#od,
});

but my other experiments were all worse; I'm open to suggestions for how to make this cleaner!

Comment on lines 131 to 136
let mode = format_ident!("{}", format!("{:?}", self.get_mode()));
let slew = format_ident!("{}", format!("{:?}", self.get_slew()));
let invert = format_ident!("{}", format!("{:?}", self.get_invert()));
let digimode =
format_ident!("{}", format!("{:?}", self.get_digimode()));
let od = format_ident!("{}", format!("{:?}", self.get_opendrain()));
Copy link
Member

Choose a reason for hiding this comment

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

can't these just be

Suggested change
let mode = format_ident!("{}", format!("{:?}", self.get_mode()));
let slew = format_ident!("{}", format!("{:?}", self.get_slew()));
let invert = format_ident!("{}", format!("{:?}", self.get_invert()));
let digimode =
format_ident!("{}", format!("{:?}", self.get_digimode()));
let od = format_ident!("{}", format!("{:?}", self.get_opendrain()));
let mode = format_ident!("{:?}", self.get_mode());
let slew = format_ident!("{:?}", self.get_slew());
let invert = format_ident!("{:?}", format!("{:?}", self.get_invert());
let digimode =
format_ident!("{:?}", self.get_digimode());
let od = format_ident!("{:?}", self.get_opendrain());

AFAICT, we shouldn't need the second string allocation?

Copy link
Member

Choose a reason for hiding this comment

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

also, it occurs to me an alternative to using the Debug impl could be implementing ToTokens for the various enums, but...that would probably just internally use syn::Ident::new or something, which would end up looking similarish. But, you could hard-code the string, which is probably either better or worse...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, the former suggestion doesn't work, spitting out an impressive 4 errors per line:

error[E0277]: the trait bound `Mode: IdentFragment` is not satisfied
   --> build/lpc55pins/src/lib.rs:131:20
    |
131 |         let mode = format_ident!("{:?}", self.get_mode());
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                    |
    |                    the trait `IdentFragment` is not implemented for `Mode`
    |                    required by a bound introduced by this call
    |
    = help: the following other types implement trait `IdentFragment`:
              &T
              &mut T
              Cow<'_, T>
              bool
              char
              proc_macro2::Ident
              std::string::String
              str
            and 6 others
    = note: required for `&Mode` to implement `IdentFragment`
note: required by a bound in `IdentFragmentAdapter`
   --> /Users/mjk/.cargo/registry/src/github.com-1ecc6299db9ec823/quote-1.0.35/src/runtime.rs:494:36
    |
494 | pub struct IdentFragmentAdapter<T: IdentFragment>(pub T);
    |                                    ^^^^^^^^^^^^^ required by this bound in `IdentFragmentAdapter`
    = note: this error originates in the macro `$crate::format_ident_impl` which comes from the expansion of the macro `format_ident` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `Mode: IdentFragment` is not satisfied
   --> build/lpc55pins/src/lib.rs:131:20
    |
131 |         let mode = format_ident!("{:?}", self.get_mode());
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `IdentFragment` is not implemented for `Mode`
    |
    = help: the following other types implement trait `IdentFragment`:
              &T
              &mut T
              Cow<'_, T>
              bool
              char
              proc_macro2::Ident
              std::string::String
              str
            and 6 others
    = note: required for `&Mode` to implement `IdentFragment`
note: required by a bound in `IdentFragmentAdapter`
   --> /Users/mjk/.cargo/registry/src/github.com-1ecc6299db9ec823/quote-1.0.35/src/runtime.rs:494:36
    |
494 | pub struct IdentFragmentAdapter<T: IdentFragment>(pub T);
    |                                    ^^^^^^^^^^^^^ required by this bound in `IdentFragmentAdapter`
    = note: this error originates in the macro `$crate::format_ident_impl` which comes from the expansion of the macro `format_ident` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: `IdentFragmentAdapter<&Mode>` doesn't implement `Debug`
   --> build/lpc55pins/src/lib.rs:131:20
    |
131 |         let mode = format_ident!("{:?}", self.get_mode());
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `IdentFragmentAdapter<&Mode>` cannot be formatted using `{:?}` because it doesn't implement `Debug`
    |
    = help: the trait `Debug` is not implemented for `IdentFragmentAdapter<&Mode>`
    = note: this error originates in the macro `$crate::__export::format_args` which comes from the expansion of the macro `format_ident` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `Mode: IdentFragment` is not satisfied
   --> build/lpc55pins/src/lib.rs:131:20
    |
131 |         let mode = format_ident!("{:?}", self.get_mode());
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `IdentFragment` is not implemented for `Mode`
    |
    = help: the following other types implement trait `IdentFragment`:
              &T
              &mut T
              Cow<'_, T>
              bool
              char
              proc_macro2::Ident
              std::string::String
              str
            and 6 others
    = note: required for `&Mode` to implement `IdentFragment`
note: required by a bound in `IdentFragmentAdapter`
   --> /Users/mjk/.cargo/registry/src/github.com-1ecc6299db9ec823/quote-1.0.35/src/runtime.rs:494:36
    |
494 | pub struct IdentFragmentAdapter<T: IdentFragment>(pub T);
    |                                    ^^^^^^^^^^^^^ required by this bound in `IdentFragmentAdapter`
    = note: this error originates in the macro `$crate::__export::format_args` which comes from the expansion of the macro `format_ident` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: the method `span` exists for struct `IdentFragmentAdapter<&Mode>`, but its trait bounds were not satisfied
   --> build/lpc55pins/src/lib.rs:131:20
    |
131 |         let mode = format_ident!("{:?}", self.get_mode());
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ method cannot be called on `IdentFragmentAdapter<&Mode>` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `&Mode: IdentFragment`
    = note: this error originates in the macro `$crate::format_ident_impl` which comes from the expansion of the macro `format_ident` (in Nightly builds, run with -Z macro-backtrace for more info)

(an easy way to test is running cargo xtask build --dirty app/oxide-rot-1/app.toml swd)

I experimented with implementing some of the syn traits, but am iffy about the amount of boilerplate required...

Copy link
Member

Choose a reason for hiding this comment

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

Oh, huh, I was apparently wrong about how format_ident! works!

I suppose another option would just be to write

syn::Ident::new(format!("{:?}", self.get_mode()), proc_macro2::Span::call_site())

to avoid the second allocation, although this is, admittedly, uglier...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm on the fence here; if all solutions are equally ugly, then might as well pick the shorter one.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah...the impact on build time is probably not meaningful. Carry on.

Some(s) => s.to_string(),
}
fn get_mode(&self) -> Mode {
self.mode.unwrap_or(Mode::NoPull)
Copy link
Collaborator

Choose a reason for hiding this comment

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

On all of these, I recommend derive(Default) on the enum with a #[default] tag on the variant you want. These become unwrap_or_default. Or, at that point, you might leave the defaulting to the caller, depending on how it's used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice, that comes out slightly shorter

@mkeeter mkeeter enabled auto-merge (squash) April 15, 2024 19:10
@mkeeter mkeeter disabled auto-merge April 15, 2024 19:11
@mkeeter mkeeter enabled auto-merge (squash) April 15, 2024 19:14
@mkeeter mkeeter merged commit ad24cfa into master Apr 15, 2024
104 checks passed
@mkeeter mkeeter deleted the stronger-codegen-types branch April 15, 2024 19:21
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.

None yet

4 participants