make 'fn ptr()' APIs to be 'const fn ptr()'

[JOE1994]

This PR changes functions like ITM::ptr(), DWT::ptr() to become const fns.

• functions like ITM::ptr(), DWT::ptr() return pointers that are cast from constants,
  but currently these functions can't be used to define a constant. This PR will allow below code to compile.

use cortex_m::peripheral::ITM;
use cortex_m::peripheral::itm::RegisterBlock;
// Below line currently won't compile, since `ITM::ptr()` is not a `const fn`
const ITM_PTR: *mut RegisterBlock = ITM::ptr();

I couldn't think of disadvantages that might accompany this change, but please correct me if I'm wrong.
Thank you for reviewing this PR 👍

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jonas-schievink]

What's the use case for something like this?

If we do this, we probably also want to change svd2rust accordingly, since it generates similar APIs.

(aside: is there any reason why ptr is not an associated constant instead?)

[JOE1994]

What's the use case for something like this?

To be honest, I don't have a solid use case for this. My idea was just "Why not make these const fns?".
Now I realized that I rather should've opened an issue first, rather than a PR. I apologize for this spammish PR 😢

If we do this, we probably also want to change svd2rust accordingly, since it generates similar APIs.

(aside: is there any reason why ptr is not an associated constant instead?)

Making ptr to be associated constants seems to be a much better option than making these functions to be const fn 😃
(although it would be a breaking change).
It would make the code more neat (in my personal opinion), and slightly reduce compile time.

This PR is rather a minor refactoring attempt that doesn't solve practical issues.
I could close this PR and open an issue for discussion instead.

[therealprof]

I think we should generally try to be more idiomatic and expressive even without particular usecase or problem to address. Declaring constant functions as const makes a lot of sense from an expressive point of view, enables new uses and could even today lead to potentially better code generation in dev mode. So I'm all for such changes, especially if they don't require a MSRV bump.

[jonas-schievink]

could even today lead to potentially better code generation in dev mode

Not sure why that would be the case here, unless we fully move to an associated constant.

[therealprof]

Not sure why that would be the case here, unless we fully move to an associated constant.

There're plenty of cases where rustc decides to use a function call instead of simply inlining the code. My understanding is that const fn are/will be evaluated during compile time in more cases and are thus less likely to end up as function calls. Seems like non-breaking low hanging fruit to me, independent of associated consts.

[Disasm]

Yet another use case (for svd2rust generated code, though):
https://github.com/stm32-rs/stm32f4xx-hal/blob/44f2f01e711981e89d031b4331b541e07643094d/src/otg_fs.rs#L29

[jonas-schievink]

My understanding is that const fn are/will be evaluated during compile time in more cases and are thus less likely to end up as function calls.

No, not at the moment. They're only evaluated at compile time if you initialize a const or static with them or use them in an array's length. rustc will not inline anything that isn't #[inline(always)] in debug mode, if people want inlining they should enable optimizations.

@Disasm's use case looks interesting. I think it makes sense to move svd2rust over to associated constants as well as this crate (perhaps with a migration period in which we provide both).

[therealprof]

As discussed in the meeting having const fn ptr() is a nice thing to have and we'll be happy to take it for now. Longer term we're going to move to associated consts and will deprecate the free ptr functions.

[adamgreig]

I think the idea was to add the associated constants now too, since they can be a non-breaking addition and are more generally useful, but then move to deprecating the ptr methods in the next breaking release (0.7).

[therealprof]

@adamgreig I shouldn't have edited the second sentence in so sloppily. 😅 Of course I meant to say we're adding in the associated const right away.

[JOE1994]

Should I add another commit to add in associated constants also??

[therealprof]

@JOE1994 Yes, please.

[JOE1994]

I think this PR is ready to be reviewed 😺

[Disasm]

I'd instead call these constants with the same name for consistency (PTR might work). However, no strong feelings about it.

[therealprof]

Yes, those should all be called PTR so they have a common name, analogous to the ptr() function.

[therealprof]

Now couldn't the ptr() functions just use the fine new PTR consts? ;)

[therealprof]

One tiny little ask and then I'm happy: Would you please add an entry to the CHANGELOG?

[JOE1994]

I did a rebase to update the CHANGELOG.

[therealprof]

I think the plan was to mark as deprecated in 0.7 and remove it for good in 1.0? Anyway I'd just leave it out for now, we can always tweak the deprecation/removal plan in a separate PR.

[therealprof]

Excellent, thanks a lot!

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push