Skip to content

Conversation

@michaelwoerister
Copy link
Member

The only requirement for debuginfo type IDs is that they are globally unique. The TypeIdHasher (which is used for std::intrinsic::type_id() provides that, so we can get rid of some redundancy by re-using it for debuginfo. Values produced by the TypeIdHasher are also more stable than the current UniqueTypeId generation algorithm produces -- these incorporate the NodeIds, which is not good for incremental compilation.

@alexcrichton @eddyb : Could you take a look at the endianess adaptations that I made to the TypeIdHasher?

Also, are we sure that a 64 bit hash is wide enough for something that is supposed to be globally unique? For debuginfo I'm using 160 bits to make sure that we don't run into conflicts there.

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

}
fn write_usize(&mut self, i: usize) {
self.inner.write_u64(i as u64)
self.inner.write_u64((i as u64).to_le())
Copy link
Contributor

Choose a reason for hiding this comment

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

This, and isize should probably not have the to_le().

Copy link
Member

Choose a reason for hiding this comment

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

Why? That would be true if they were calling self.write_u64 and not self.inner.write_u64.

Copy link
Member

Choose a reason for hiding this comment

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

Really? I'm not sure I follow why (they call the inner write_u64 at least rather than the outer one)... Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, didn't read that code carefully enough 😞 I still remembered that usize would delegate to u64 and didn't expect to go directly to the inner.

@alexcrichton
Copy link
Member

👍

}
fn write_usize(&mut self, i: usize) {
self.inner.write_u64(i as u64)
self.inner.write_u64((i as u64).to_le())
Copy link
Member

Choose a reason for hiding this comment

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

Why? That would be true if they were calling self.write_u64 and not self.inner.write_u64.

struct WidenUsizeHasher<H> {
//
// The same goes for endianess: We always convert multi-byte integers to little
// endian before hashing.
Copy link
Member

Choose a reason for hiding this comment

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

Can we turn this into a doc comment? Maybe even make it public, seems useful elsewhere.

@michaelwoerister
Copy link
Member Author

Fixed the failing test case.

@eddyb
Copy link
Member

eddyb commented Oct 24, 2016

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 24, 2016

📌 Commit 025b27d has been approved by eddyb

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Oct 24, 2016
…, r=eddyb

debuginfo: Use TypeIdHasher for generating global debuginfo type IDs.

The only requirement for debuginfo type IDs is that they are globally unique. The `TypeIdHasher` (which is used for `std::intrinsic::type_id()` provides that, so we can get rid of some redundancy by re-using it for debuginfo. Values produced by the `TypeIdHasher` are also more stable than the current `UniqueTypeId` generation algorithm produces -- these incorporate the `NodeId`s, which is not good for incremental compilation.

@alexcrichton @eddyb : Could you take a look at the endianess adaptations that I made to the `TypeIdHasher`?

Also, are we sure that a 64 bit hash is wide enough for something that is supposed to be globally unique? For debuginfo I'm using 160 bits to make sure that we don't run into conflicts there.
bors added a commit that referenced this pull request Oct 24, 2016
Rollup of 7 pull requests

- Successful merges: #37228, #37304, #37324, #37328, #37336, #37349, #37372
- Failed merges:
@bors bors merged commit 025b27d into rust-lang:master Oct 25, 2016
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.

6 participants