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

feat: borsh serde #525

Merged
merged 2 commits into from
Jun 7, 2024
Merged

feat: borsh serde #525

merged 2 commits into from
Jun 7, 2024

Conversation

dzmitry-lahoda
Copy link
Contributor

No description provided.

@dzmitry-lahoda
Copy link
Contributor Author

not clear how next relates to my PR

Compiling rkyv_derive v0.7.44
error: bound is defined in more than one place
    --> src/map.rs:1267:30
     |
[126](https://github.com/rust-lang/hashbrown/actions/runs/9102104127/job/25020927498?pr=525#step:3:127)7 |     pub fn entry_ref<'a, 'b, Q: ?Sized>(&'a mut self, key: &'b Q) -> EntryRef<'a, 'b, K, Q, V, S, A>
     |                              ^
1268 |     where
1269 |         Q: Hash + Equivalent<K>,
     |         ^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations
     = note: `-D clippy::multiple-bound-locations` implied by `-D clippy::all`
     = help: to override `-D clippy::all` add `#[allow(clippy::multiple_bound_locations)]`

error: bound is defined in more than one place
    --> src/map.rs:[130](https://github.com/rust-lang/hashbrown/actions/runs/9102104127/job/25020927498?pr=525#step:3:131)8:16
     |
1308 |     pub fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V>
     |                ^
1309 |     where
[131](https://github.com/rust-lang/hashbrown/actions/runs/9102104127/job/25020927498?pr=525#step:3:132)0 |         Q: Hash + Equivalent<K>,

@dzmitry-lahoda
Copy link
Contributor Author

neither my code nor reference is on be default

cargo clippy --all --tests --features serde,rayon -- -D clippy::all

magic

@Amanieu
Copy link
Member

Amanieu commented May 30, 2024

The CI failure should be fixed now, can you rebase your branch?

@Amanieu
Copy link
Member

Amanieu commented Jun 7, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 7, 2024

📌 Commit 734191c has been approved by Amanieu

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 7, 2024

⌛ Testing commit 734191c with merge 2310a95...

@bors
Copy link
Contributor

bors commented Jun 7, 2024

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 2310a95 to master...

@bors bors merged commit 2310a95 into rust-lang:master Jun 7, 2024
24 checks passed
@kayabaNerve
Copy link

As a policy question, why was borsh added as a dependency to hashbrown instead of borsh adding a dependency of hashbrown (and implementing on that end)?

@kayabaNerve
Copy link

... borsh already implements support for hashbrown under the hashbrown feature flag??? This prevents borsh from supporting any hashbrown >= 0.15 due to the circular dependency it'd cause. borsh did future-proof by only supporting the range they know they can support (< 0.15 is one of the bounds), so I don't immediately believe this will cause issues for anyone who updated to hashbrown 0.15 (as borsh should cause the dependency to be duplicated and maintain the <0.15 instance), yet I truly don't understand why this PR exists in the first place.

#554 demonstrates moving such functionality out of this lib and yet this PR explicitly moves it into this lib.

@Amanieu
Copy link
Member

Amanieu commented Oct 10, 2024

There's no particular policy for where trait impls should go, but generally it should be in the crate that most frequently releases major versions with breaking changes.

There are some exceptions:

  • If one crate depends on the other, make sure we don't end up with circular dependencies (this happened with rkyv).
  • rayon support needs deep integration into the implementation details of RawTable. This cannot be done in rayon itself.

@kayabaNerve
Copy link

ACK re: policy and rkyv. I'd argue that by necessity is primary, yet I'd argue for going by niche rather than by release cycle. Smaller crates should expand their functionality rather than expecting larger crates to do so to prevent overwhelming the larger crates (preventing a theoretical future where there's 100 different codecs in hashbrown instead of 1 hashbrown feature in 100 different codecs).

bors added a commit that referenced this pull request Oct 12, 2024
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