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

Mark some functions in Instruction and public api as const. #237

Open
i509VCB opened this issue Dec 5, 2021 · 10 comments
Open

Mark some functions in Instruction and public api as const. #237

i509VCB opened this issue Dec 5, 2021 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers Rust

Comments

@i509VCB
Copy link
Contributor

i509VCB commented Dec 5, 2021

Essentially introduce const where appropriate for the public types exposed by the api.

This request intentionally only targets some functions since we don't have stable &mut in const contexts along with a bunch of other const stuff that is still nightly.

I don't expect that iced will ever become 100% const, that's unreasonable to assume due to the fact iced needs liballoc around the crate. The reason for introducing const where appropriate would be some nice utility for libraries using iced downstream that don't need to use the decoder and instead just operate on already existing types such as Instruction so their functions could also be const.

@i509VCB i509VCB changed the title Mark some functions in Instruction and pubic api as const. Mark some functions in Instruction and public api as const. Dec 5, 2021
@wtfsck
Copy link
Member

wtfsck commented Dec 6, 2021

Yes, many/most/all of those methods could be const. Want to send a PR and enable those you need (and maybe more?)

@wtfsck wtfsck closed this as completed Dec 6, 2021
@wtfsck wtfsck added enhancement New feature or request good first issue Good for newcomers Rust labels Dec 6, 2021
@wtfsck wtfsck reopened this Dec 6, 2021
@wtfsck
Copy link
Member

wtfsck commented Dec 6, 2021

Didn't mean to close it.

@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 6, 2021

Sure I could send a PR, probably won't be till Friday though.

@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 17, 2021

Doing some work on this, some things that should unlock more parts of the library that can be made const for the future.

  1. Lookup tables are static, not sure if that is intentional or if the lookup tables could be made const.
    • Notably this blocks const for Instruction::mnemonic.
  2. MSRV upgrades will make more things const. Specifically 1.56 made mem::transmute const.
    • This is something for wtfsck to decide when to do.

And the stuff out of our control and for the language to languish in.

  1. impl const for traits is not ready yet, this will allow quite a few things to be made const.
    • Namely all the impls of PartialEq, Eq, PartialOrd, Ord, Default, Into, etc and anything blocked by relying on those impls.
  2. &mut in const contexts, nightly at the moment I believe, this would make many of the setter functions const.

@wtfsck
Copy link
Member

wtfsck commented Dec 17, 2021

Other Instruction methods that could be made const:

  • new ctor
  • setters, so far only the getters have been made const (EDIT: apparently not possible yet according to your post)

Also the with*() fns can't be made const yet as mentioned in your post above. But with const new and const setters it's possible to do it manually.

@wtfsck
Copy link
Member

wtfsck commented Dec 17, 2021

Lookup tables are static, not sure if that is intentional or if the lookup tables could be made const.

Many of them definitely can be made const, unless it results in slow compile time or bigger binary file size or something like that.

@wtfsck
Copy link
Member

wtfsck commented Dec 17, 2021

MSRV upgrades will make more things const. Specifically 1.56 made mem::transmute const

1.56.0 version is too recent, current version is 1.57.0. That will probably prevent too many from using the crate.

@i509VCB
Copy link
Contributor Author

i509VCB commented Dec 17, 2021

Other Instruction methods that could be made const:

  • new ctor
  • setters, so far only the getters have been made const (EDIT: apparently not possible yet according to your post)

Also the with*() fns can't be made const yet as mentioned in your post above. But with const new and const setters it's possible to do it manually.

Yep I can probably do some new constructors and just delegate Default to those. My first pull request was really for everything that is blindingly obvious that could be made const.

For &mut const, that is still nightly: rust-lang/rust#57349

@not-matthias
Copy link

Many of them definitely can be made const, unless it results in slow compile time or bigger binary file size or something like that.

I changed all the lookup tables locally to const and I didn't notice any problems. The compilation time took, instead of 24s, about 26s (so 2 seconds slower). The file size was also only 1kb smaller.

@i509VCB
Copy link
Contributor Author

i509VCB commented Jan 29, 2022

I've learned of the clippy::missing_const_for_fn lint. This could be useful for determining what could be made const.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Rust
Projects
None yet
Development

No branches or pull requests

3 participants