Skip to content

Make naga span methods take path as generic AsRef Path #7643

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

atlv24
Copy link
Collaborator

@atlv24 atlv24 commented Apr 28, 2025

Connections
none

Description
These methods dont match naga/src/front/wgsl/error.rs and are harder to use than them.

Testing
It compiles.

Squash or Rebase?

Doesn't matter

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

lgtm yo

…minus a teeny nit. 🙂 Approving conditional on resolution.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Ahhh, but wait, just checked CI failures. The Windows one looks spurious, but the wasm32 build failure is a problem. These APIs exposed in no_std environments; no_std means no std::path::Path1. 💦 We need to resolve this conflict to proceed.

😐 Sorry for the whiplash!

Footnotes

  1. That seems counter-intuitive with the stderr name—CC @bushrat011899!

@bushrat011899
Copy link
Contributor

Re. no_std: I've created a little Gist here that I think solves the issue quite cleanly. The idea is to create a new trait, AsPath, which exposes the methods you need (e.g., to_string_lossy) and either:

  • Blanket implement for anything implementing AsRef<Path> when std is available; or
  • Specifically implement for the subset of types that implement AsRef<Path> but are available without std (e.g., String and str).

This ensures the std feature is still additive and gives the same ergonomic benefit to std users, and gives a lesser (but still positive) boost to no_std too.

@atlv24 atlv24 requested a review from a team as a code owner May 8, 2025 08:45
@atlv24 atlv24 requested a review from ErichDonGubler May 8, 2025 09:47
@cwfitzgerald
Copy link
Member

@bushrat011899 To be extremely pedantic and annoying, you need it to be a sealed trait for it to be fully additive - otherwise users could do impl AsPath for MyType where MyType: AsRef<Path> and it's fine with the std feature but not fine without it.

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.

4 participants