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

Expand const fn coverage #319

Merged
merged 4 commits into from
Jan 27, 2020
Merged

Expand const fn coverage #319

merged 4 commits into from
Jan 27, 2020

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Jan 27, 2020

Follow up to #318

@parity-cla-bot
Copy link

It looks like @vorot93 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@dvdplm
Copy link
Contributor

dvdplm commented Jan 27, 2020

What are your thoughts on what criteria to use for deciding what should be const and what shouldn't? E.g. in uint there are a few methods left non-const: low_u128(), bit(), split(), split_u128() – what was the reasoning there?

Another question I have is about the removed #[inline]s: I thought const fn meant it can be transformed to a constant at compile time, but is not guaranteed to be. In other words, that in some code it'd be constified but other times called as a normal function. Am I mistaken?

@vorot93
Copy link
Contributor Author

vorot93 commented Jan 27, 2020

@dvdplm my idea is to make pretty much everything const fn unless it is not allowed to be such. That means that the most basic of methods with no mutation can become const fn today.

I missed the methods you listed and will make them const fn right away.

re: inline - we would need a compiler expert here, but I'd expect any function that uses const fn to be classified by internal heuristics as potentially inline-able. This is not something I'm completely sure about though and can restore the attribute.

@dvdplm
Copy link
Contributor

dvdplm commented Jan 27, 2020

I missed the methods you listed and will make them const fn right away.

Got it, cool. It would be great to have a tool/lint to help us find code that can be constified. :)

@dvdplm
Copy link
Contributor

dvdplm commented Jan 27, 2020

re: inline - we would need a compiler expert here, but I'd expect any function that uses const fn to be classified by internal heuristics as potentially inline-able. This is not something I'm completely sure about though and can restore the attribute.

Reading up a little about this, my understanding is that a const fn implies a guarantee that the function is callable in a const context; when called in a non-const context the const fn is called like a normal function. E.g. given:

const fn is_even(x: usize) -> bool { x % 2 == 0 }

…then is_even(42) is guaranteed to be const, but if I have fn random() -> usize somewhere and call is_even(random()) then it is not folded.

My conclusion is that we want to keep the #[inline].

@vorot93
Copy link
Contributor Author

vorot93 commented Jan 27, 2020

@dvdplm thank you for the write-up. I have added back #[inline] in the last commit.

@ordian ordian merged commit de318bf into paritytech:master Jan 27, 2020
@vorot93
Copy link
Contributor Author

vorot93 commented Jan 27, 2020

By the way, there is an actual clippy lint for potential const fn now :)
https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn

@vorot93 vorot93 deleted the more-const-fn branch January 27, 2020 22:45
dvdplm added a commit that referenced this pull request Feb 4, 2020
* master:
  update changelogs (#329)
  bump parity-util-mem to 0.4.2 (#328)
  remove libc feature from fixed-hash (#317)
  kvdb-rocksdb: release 0.4.2 (#327)
  kvdb-rocksdb: fix iter_from_prefix being slow (#326)
  MallocSizeOf for BTreeSet (#325)
  split off primitives (#323)
  travis: disable kvdb-web tests for chrome (#324)
  Expand const fn coverage (#319)
  uint: make zero const fn (#318)
  README: fix appveyor badge (#316)
  keccak-hash: switch benches to criterion (#315)
ordian added a commit that referenced this pull request Feb 6, 2020
* master: (56 commits)
  Remove libc completely (#333)
  update changelogs (#329)
  bump parity-util-mem to 0.4.2 (#328)
  remove libc feature from fixed-hash (#317)
  kvdb-rocksdb: release 0.4.2 (#327)
  kvdb-rocksdb: fix iter_from_prefix being slow (#326)
  MallocSizeOf for BTreeSet (#325)
  split off primitives (#323)
  travis: disable kvdb-web tests for chrome (#324)
  Expand const fn coverage (#319)
  uint: make zero const fn (#318)
  README: fix appveyor badge (#316)
  keccak-hash: switch benches to criterion (#315)
  update parity-util-mem (#309)
  Update features and feature dependencies (#307)
  Use proper memory queries to rocksdb (#308)
  Draft version updates and changelog (#299)
  Use custom error type for `from_hex` (#305)
  Fix typo. (#303)
  kvdb: remove KeyValueDBHandler (#304)
  ...
ordian added a commit that referenced this pull request Feb 7, 2020
* master:
  Add different mode for malloc_size_of_is_0 macro dealing with generics (#334)
  [parity-crypto] Use upstream secp256k1 (#258)
  Bump parking_lot to 0.10 and minor versions (#332)
  Remove libc completely (#333)
  update changelogs (#329)
  bump parity-util-mem to 0.4.2 (#328)
  remove libc feature from fixed-hash (#317)
  kvdb-rocksdb: release 0.4.2 (#327)
  kvdb-rocksdb: fix iter_from_prefix being slow (#326)
  MallocSizeOf for BTreeSet (#325)
  split off primitives (#323)
  travis: disable kvdb-web tests for chrome (#324)
  Expand const fn coverage (#319)
  uint: make zero const fn (#318)
  README: fix appveyor badge (#316)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants