Skip to content

Conversation

@jannic
Copy link
Member

@jannic jannic commented Jul 1, 2025

Fixes #321

I intentionally didn't update the indentation, for two reasons:

  1. I don't even know how this should be indented
  2. This way it's easier to review

If you'd prefer some certain way of indenting the conditional blocks inside the cfg_global_asm! call, I'll happily update the pull request.

@jannic jannic requested a review from a team as a code owner July 1, 2025 20:50
@jannic jannic force-pushed the single-global-asm-macro branch from 0c2a208 to e320adc Compare July 1, 2025 20:58
Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

@romancardenas
Copy link
Contributor

Minor clippy fix:

https://github.com/rust-embedded/riscv/actions/runs/16010020609/job/45165521521?pr=322

Otherwise, LGTM.

is_multiple_of came with Rust 1.87: https://doc.rust-lang.org/std/primitive.usize.html#method.is_multiple_of

Bumping the MSRV to make Nightly Clippy happy seems too much. Currently, Nightly builds and checks are marked as experimental, and it is OK to merge PRs with reasonable failures in Nightly.

I'm CC'ing @gibbz00 , as he mentioned that it might be possible to "relax" a bit Nightly clippy requirements

Copy link
Contributor

@romancardenas romancardenas left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

There are a few core::arch::global_asm! calls in the macros and at the end of asm.rs, but all of these blobs specify a section, so I guess it is not UB? We don't need them to be in a specific memory location or order, as long as they are in their corresponding section. In this case, the linker will do the hard work.

@gibbz00
Copy link
Contributor

gibbz00 commented Jul 2, 2025

I'm CC'ing @gibbz00 , as he mentioned that it might be possible to "relax" a bit Nightly clippy requirements

clippy::manual_is_multiple_of is unfortunately part of the "complexity" lint group, whose default lint level is "warn". I would suggest adding an #[allow(clippy::manual_is_multiple_of, reason = "requires MSRV bump")] for now. Relaxing clippy lints would entail removing this line:

CLIPPY_PARAMS: -W clippy::all -W clippy::pedantic -W clippy::nursery -W clippy::cargo

(Concise lint group explanation here: https://github.com/rust-lang/rust-clippy?tab=readme-ov-file#clippy)

That is, the general lint policy should IMO be;

  • Avoid opting-out of default lints unless there's a strong reason for it. (Like the one above.)
  • Selectively opt-in to non-default/allow-by-default lints at the lint level, not lint group level. Ex. by adding -W missing-docs.

@romancardenas romancardenas added this pull request to the merge queue Jul 3, 2025
Merged via the queue into rust-embedded:master with commit afa79b5 Jul 3, 2025
137 of 139 checks passed
@jannic
Copy link
Member Author

jannic commented Jul 3, 2025

Thanks for discussing the clippy warnings. I didn't have time to care about them yesterday. I agree that it doesn't make sense chasing every single new warning on nightly immediately.

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.

asm.rs assumes that multiple global_asm statements are concatenated

4 participants