Can derive hash analysis#887
Can derive hash analysis#887bors-servo merged 12 commits intorust-lang:masterfrom photoszzt:derive_hash
Conversation
|
☔ The latest upstream changes (presumably #866) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #859) made this pull request unmergeable. Please resolve the merge conflicts. |
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Overall, this looks great, thanks!
I'm impressed with your tenacity in going through all those existing tests and enabling deriving hash for them.
However, what I was expecting, and what is missing from this PR, is a handful of small, new tests targeted specifically at exercising the edge cases of when we can or cannot derive hash. The change added to the function pointer test is a good start, but I'd like to also see:
- When the layout for an opaque type is too large to derive hash
- When the layout for an opaque type can derive hash
- A struct containing a struct containing a float that cannot derive hash
- A struct containing an array of floats that cannot derive hash
- A test with structs containing mutable and const pointers not deriving and deriving hash respectively
- A test with a template definition containing a float, which cannot derive hash
- A test with a template that does not contain a float, and can derive hash when instantiated with
intbut cannot derive hash when instantiated withfloat. - A test where a type that we cannot derive hash for is blacklisted
- A test where a type that we could have derived hash for is blacklisted
Thanks!
src/lib.rs
Outdated
| } | ||
|
|
||
| if !self.options.derive_hash { | ||
| output_vector.push("--no-derive-hash".into()); |
There was a problem hiding this comment.
There is no such flag defined, and the default is not to derive hash, so this branch should be removed and we should only have the --with-derive-hash case.
| let union_field_hash_impl = quote_item!(&ctx.ext_cx(), | ||
| impl<T> ::$prefix::hash::Hash for __BindgenUnionField<T> { | ||
| fn hash<H: ::$prefix::hash::Hasher>(&self, _state: &mut H) { | ||
| } |
There was a problem hiding this comment.
This needs a comment that the actual physical storage will be hashed, and that's why these fields don't actually do anything with the hash. That's a very surprising impl to glance at without knowing the larger context.
|
|
I'm sorry I didn't make my feedback clearer in the original comment, and we've had to go back and forth here.
// bindgen-flags: --with-derive-hash --whitelist-type 'Whitelisted.*' --blacklist-type Blacklisted
template<class T>
struct Blacklisted {
T t;
};
/// This would derive(Hash) if it didn't contain a blacklisted type,
/// causing us to conservatively avoid deriving hash for it.
struct WhitelistedOne {
Blacklisted<int> a;
};
/// This can't derive(Hash) even if it didn't contain a blacklisted type.
struct WhitelistedTwo {
Blacklisted<float> b;
};
Then we should add comments to those test headers so that if/when their output changes in the future, the author of that change can determine whether it is valid or not, or if it is a regression. Otherwise, they would have to do lots of tedious code archaeology to determine this. |
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
This is great, thanks for your patience with me!
👍
|
@bors-servo r+ |
|
📌 Commit b4895d9 has been approved by |
|
☀️ Test successful - status-travis |
3 participants
r? @fitzgen Fix: #876 I haven't enable too much test yet. Enable the option for all tests changes more than 200 of them. I want some input on which test is good to enable or write a new test.