Skip to content

feat: borsh serde#525

Merged
bors merged 2 commits intorust-lang:masterfrom
dzmitry-lahoda-forks:dz/1
Jun 7, 2024
Merged

feat: borsh serde#525
bors merged 2 commits intorust-lang:masterfrom
dzmitry-lahoda-forks:dz/1

Conversation

@dzmitry-lahoda
Copy link
Copy Markdown
Contributor

No description provided.

@dzmitry-lahoda
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Amanieu commented May 30, 2024

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

@Amanieu
Copy link
Copy Markdown
Member

Amanieu commented Jun 7, 2024

@bors r+

@bors
Copy link
Copy Markdown
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
Copy Markdown
Contributor

bors commented Jun 7, 2024

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

@bors
Copy link
Copy Markdown
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
@kayabaNerve
Copy link
Copy Markdown

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
Copy Markdown

... 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
Copy Markdown
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
Copy Markdown

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