Skip to content

Conversation

@alilleybrinker
Copy link
Contributor

The prior macro expansion could produce errors if the macros were called in a context where Result is redefined, for example in a crate with its own Result type which pre-fills the error type. This replaces existing Result uses with std::result::Result to avoid the compilation error in that case.

@netlify
Copy link

netlify bot commented Nov 18, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 50da4a7
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6926ac2c9428d200086cfb29

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 18, 2025

CodSpeed Performance Report

Merging #1025 will improve performances by 7.07%

Comparing alilleybrinker:patch-1 (50da4a7) with master (17bc55d)

Summary

⚡ 1 improvement
✅ 12 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[Input] 2.2 µs 2.1 µs +7.07%

@MichaReiser
Copy link
Contributor

Thank you. Would you mind adding a test so that we catch future regressions early

@alilleybrinker
Copy link
Contributor Author

@MichaReiser sure, where would you want tests to be for this? Cursory review of the repo didn't show an obvious place with existing tests I should be augmenting. Want to make sure I'm as consistent with any existing practice as possible.

@alilleybrinker
Copy link
Contributor Author

To maybe be clearer, I think the test here would be a compilation success test, validating that the relevant macros can compile even if someone redefines Result in the importing module. At the moment I only see compile-fail tests. I'm happy to add a compile-success folder and put this in it, but wanted to check first.

The prior macro expansion could produce errors if the macros were
called in a context where `Result` is redefined, for example in a
crate with its own `Result` type which pre-fills the error type. This
replaces existing `Result` uses with `std::result::Result` to avoid
the compilation error in that case.

Signed-off-by: Andrew Lilley Brinker <[email protected]>
@alilleybrinker
Copy link
Contributor Author

Added a new "compile pass" test, and from it found some more spots that needed their use of Result to be fully qualified. Let me know if you want any more edits.

@MichaReiser MichaReiser added this pull request to the merge queue Nov 26, 2025
@MichaReiser
Copy link
Contributor

Thank you

Merged via the queue into salsa-rs:master with commit 59aa107 Nov 26, 2025
12 checks passed
@alilleybrinker alilleybrinker deleted the patch-1 branch November 26, 2025 15:51
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.

2 participants