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

Add RcBuf variant to DataPayload #816

Merged
merged 20 commits into from
Jun 23, 2021
Merged

Add RcBuf variant to DataPayload #816

merged 20 commits into from
Jun 23, 2021

Conversation

sffc
Copy link
Member

@sffc sffc commented Jun 19, 2021

See #667

@sffc
Copy link
Member Author

sffc commented Jun 19, 2021

I added a new variant to DataPayload for buffers and wired it up in FsDataProvider and StaticDataProvider. I used the following bound on functions where the data struct needs to be deserializable:

for<'de> <M::Yokeable as Yokeable<'de>>::Output: serde::de::Deserialize<'de>,

Good news:

  • The core library code compiles
  • I did some interesting stuff to make SerdeDeDataProvider compile, including a double-callback function
  • Somehow, I am using the proper attach_to_cart and it compiles

Bad news:

  • Call sites of FsDataProvider and StaticDataProvider do not compile, giving errors such as "the trait for<'de> serde::de::Deserialize<'de> is not implemented for <DecimalSymbolsV1 as Yokeable<'de>>::Output"

I believe this is rust-lang/rust#85636 again. Unfortunately, I don't think I can use the "reference workaround" here again, because I care about Self (like I do with Clone).

@Manishearth Thoughts?

UPDATE: Manish answered my question and posted the solution in #85636.

provider/core/src/serde.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #816 (904bd08) into main (a7a9650) will increase coverage by 0.11%.
The diff coverage is 62.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #816      +/-   ##
==========================================
+ Coverage   75.32%   75.43%   +0.11%     
==========================================
  Files         192      193       +1     
  Lines       12374    12499     +125     
==========================================
+ Hits         9321     9429     +108     
- Misses       3053     3070      +17     
Impacted Files Coverage Δ
experimental/provider_static/src/lib.rs 4.00% <0.00%> (ø)
ffi/capi/src/provider.rs 0.00% <0.00%> (ø)
provider/core/src/dynutil.rs 38.88% <ø> (ø)
provider/core/src/serde.rs 47.05% <0.00%> (-12.95%) ⬇️
utils/yoke/src/lib.rs 100.00% <ø> (ø)
utils/yoke/src/trait_hack.rs 0.00% <0.00%> (ø)
utils/yoke/src/yokeable.rs 81.81% <ø> (-18.19%) ⬇️
provider/fs/src/deserializer.rs 37.50% <52.63%> (-3.41%) ⬇️
provider/core/src/data_provider.rs 70.31% <54.54%> (-3.28%) ⬇️
provider/core/src/erased.rs 82.88% <85.36%> (+15.74%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7a9650...904bd08. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 23, 2021

Pull Request Test Coverage Report for Build f0b79ddad05faf2b5d1be5e40ef90cd90c1e61e4-PR-816

  • 78 of 124 (62.9%) changed or added relevant lines in 9 files are covered.
  • 21 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.1%) to 75.508%

Changes Missing Coverage Covered Lines Changed/Added Lines %
experimental/provider_static/src/lib.rs 0 1 0.0%
ffi/capi/src/provider.rs 0 3 0.0%
utils/yoke/src/trait_hack.rs 0 4 0.0%
provider/core/src/data_provider.rs 6 11 54.55%
provider/core/src/erased.rs 35 41 85.37%
provider/fs/src/deserializer.rs 10 19 52.63%
provider/core/src/serde.rs 0 18 0.0%
Files with Coverage Reduction New Missed Lines %
experimental/provider_ppucd/src/support.rs 1 58.49%
provider/fs/src/deserializer.rs 1 37.5%
provider/fs/src/fs_data_provider.rs 1 89.83%
components/uniset/src/provider.rs 2 52.94%
tools/datagen/src/main.rs 2 33.33%
utils/yoke/src/yokeable.rs 2 81.82%
provider/core/src/dynutil.rs 12 38.89%
Totals Coverage Status
Change from base Build bac781dee970092ae7b953e08ae9f9c69a267524: 0.1%
Covered Lines: 9557
Relevant Lines: 12657

💛 - Coveralls

provider/fs/src/fs_data_provider.rs Outdated Show resolved Hide resolved
provider/core/src/serde.rs Outdated Show resolved Hide resolved
@sffc sffc marked this pull request as ready for review June 23, 2021 07:58
@sffc sffc requested a review from a team as a code owner June 23, 2021 07:58
@sffc sffc requested review from Manishearth and removed request for a team June 23, 2021 07:58
Manishearth
Manishearth previously approved these changes Jun 23, 2021
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good! Glad to see this coming together so nicely.

T: serde::Deserialize<'de>,
M: DataMarker<'s>,
M::Yokeable: serde::de::Deserialize<'static>,
for<'de> SerdeDeDataStructWrap<<M::Yokeable as Yokeable<'de>>::Output>:
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Please add a comment with the actual bound we are trying to express here (and a link to the issue)

(ditto everywhere else the hack wrap is used)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 4 places and I made them all look like this:

    // Actual bound:
    //     for<'de> <M::Yokeable as Yokeable<'de>>::Output: serde::de::Deserialize<'de>,
    // Necessary workaround bound (see yoke docs):
    for<'de> YokeTraitHack<<M::Yokeable as Yokeable<'de>>::Output>: serde::de::Deserialize<'de>,

provider/fs/src/fs_data_provider.rs Outdated Show resolved Hide resolved
@sffc sffc requested a review from Manishearth June 23, 2021 18:49
@sffc
Copy link
Member Author

sffc commented Jun 23, 2021

I added yoke::trait_hack with docs and examples.

@sffc
Copy link
Member Author

sffc commented Jun 23, 2021

Manishearth
Manishearth previously approved these changes Jun 23, 2021
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

small nit, r=me otherwise

provider/core/src/serde.rs Outdated Show resolved Hide resolved
@sffc sffc merged commit 2b7ca3b into unicode-org:main Jun 23, 2021
@sffc sffc deleted the rc_buffer branch June 23, 2021 22:04
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