-
Notifications
You must be signed in to change notification settings - Fork 2
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
Impl from collections for RpcValue
#6
Merged
fvacek
merged 9 commits into
silicon-heaven:master
from
j4r0u53k:impl-from-collections-for-rpcvalue
Jul 8, 2024
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
5d6034a
Implement `From` on generic collections for `RpcValue`
j4r0u53k 9aed73c
Add tests for conversions from collections<T> into RpcValue
j4r0u53k 69e68cc
rpcvalue: Initialize map in tests by collect from slice
j4r0u53k 2cc8a19
Introduce `specialization` feature
j4r0u53k e58df3b
CI: Add stable and nightly toolchains to GitHub workflow
j4r0u53k 6090f15
Fix useless conversion to the same type: `rpcvalue::Value`
j4r0u53k 2584371
tests: Remove unused import: `shvproto::rpcvalue`
j4r0u53k 4cbe6eb
rpcvalue: Separate code using specialization into distinct module
j4r0u53k 5a92392
Add tests for `.into()` RpcValue on collections
j4r0u53k File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I which production project can we use unstable rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's entirely up to consumers of the lib whether they want to enable this feature or not. By default it's disabled.
The purpose of this feature is only to provide optimalization for converting a collection of
RpcValue
s intoRpcValue
, where a user code decides to handle all the conversions in the uniform way - call.into()
on any convertible data without special treatment ofCollection<RpcValue>
case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have only one consumer currently - us. So this code is effectively dead. We can keep it until
min_specialization
will be stabilized, hopefully there will be some external consumers this time. My concern was just to know, how can be this feature utilized now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether or not there are currently any external consumers shouldn't be the concern here. Anybody can start a project at any time utilizing the feature if they consider it meaningful.
The
min_specialization
has been around for a while and it's highly demanded. I don't think it will get removed. However, it might not get stabilized for another couple of years, which would mean we should either give up on this feature completely, or at least leave it there as an option, disabled by default, which is what I propose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can keep it and revisit it when
specialization
will be stabilized even if it is far from finished rust-lang/rust#31844.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still wonder, why specialization generates necessity of macro used on lines
rpcvalue.rs:208
-rpcvalue.rs:222
, which wasn't needed before. My naive understanding is, that just special cases should be introduced, but the genericimpl
should remain the same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
cfg
macros inside theimpl
s are necessary because of thedefault
keyword/marker in the definition of the generic implementations offrom
methods, which can appear only if the compiler feature is enabled.