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

Manual Debug impl for salsa::Id #508

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

MichaReiser
Copy link
Contributor

I've often been confused by the fact that the IDs printed by DebugWithDb don't match with the ids printed when dumping a salsa::Id with Debug.

This PR replaces the derived Debug implementation of salsa::Id with a manual Debug implementation that ensures that the IDs printed by Debug and DebugWithdb are consistent.

Open Questions

  • Should Id use f.debug_tuple to preserve the name in the Debug output?
  • Should DebugWithDb continue to use as_u32 directly or should it just call the Id's Debug implementation (It would change the output if we decide that Debug should output Id(num).

Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 6975a47
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/669e8126dc6aa80008ff9f0f

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Thoughts?

src/id.rs Outdated
@@ -40,6 +40,12 @@ impl Id {
}
}

impl std::fmt::Debug for Id {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
self.as_u32().fmt(f)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think it would be better to print something other than just a number -- e.g., Id(N). I've usually found there is value in having some more "grep-able".

@nikomatsakis
Copy link
Member

* Should `Id` use `f.debug_tuple` to preserve the name in the `Debug` output?

I think yes :)

* Should `DebugWithDb` continue to use `as_u32` directly or should it just call the `Id`'s `Debug` implementation (It would change the output if we decide that `Debug` should output `Id(num)`.

I like that it uses the debug impl, keep that I think.

@MichaReiser MichaReiser force-pushed the id-manual-debug-impl branch 3 times, most recently from ff0bcd6 to 662c58f Compare June 23, 2024 16:58
Comment on lines 58 to 60
[salsa id]: Id(
0,
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, the fact that debug_tuple splits the ID across multiple lines in alternate mode is too verbose. I'm just gonna use a regular write call.

@MichaReiser MichaReiser force-pushed the id-manual-debug-impl branch from 53c3cf7 to 94f36f0 Compare June 23, 2024 17:01
@MichaReiser MichaReiser requested a review from nikomatsakis June 23, 2024 17:02
@MichaReiser MichaReiser force-pushed the id-manual-debug-impl branch from 94f36f0 to 5751cb3 Compare June 23, 2024 17:06
@MichaReiser
Copy link
Contributor Author

I updated the Debug implementation to write Id({}).

@MichaReiser
Copy link
Contributor Author

@nikomatsakis I addressed the feedback. Would you mind taking another look?

@MichaReiser
Copy link
Contributor Author

Ha, I probably need to start over. @nikomatsakis I don't mind rebasing (re-doing) if this is something we want to merge.

@nikomatsakis
Copy link
Member

I'm in favor of this but needs a rebase

@MichaReiser MichaReiser force-pushed the id-manual-debug-impl branch 2 times, most recently from fc3d39d to ff23927 Compare July 22, 2024 13:27
@MichaReiser
Copy link
Contributor Author

I'm in favor of this but needs a rebase

Done

@MichaReiser MichaReiser force-pushed the id-manual-debug-impl branch from ff23927 to 6975a47 Compare July 22, 2024 15:56
@nikomatsakis nikomatsakis added this pull request to the merge queue Jul 22, 2024
Merged via the queue into salsa-rs:master with commit f5f21cf Jul 22, 2024
7 of 8 checks passed
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