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

Make types like Box<dyn DynAccess> implement Access #78

Conversation

steffahn
Copy link
Contributor

@steffahn steffahn commented Jul 5, 2022

by generalizing the P: Deref-implementation to ?Sized,
and implementing Access for some dyn DynAccess trait object types.

Not yet documented or polished, this PRs main purpose for now is to allow CI testing.

by generalizing the P: Deref-implementation to ?Sized,
and implementing Access for some `dyn DynAccess` trait object types.

Not yet documented or polished, this commit's main purpose is to allow CI testing.
@steffahn steffahn force-pushed the box-dyn-dynaccessconvert-implements-access branch from bc3b4ff to a113002 Compare July 5, 2022 18:25
@steffahn steffahn changed the title Make types like Box<dyn DynAccess> implementn Access Make types like Box<dyn DynAccess> implement Access Jul 5, 2022
@steffahn
Copy link
Contributor Author

steffahn commented Jul 5, 2022

The failing clippy lints are unrelated to this PR.

@steffahn
Copy link
Contributor Author

steffahn commented Jul 5, 2022

Older compiler version apparently fails in one of the dependencies (half).

@codecov-commenter
Copy link

Codecov Report

Merging #78 (a113002) into master (f7f192d) will increase coverage by 0.60%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   83.37%   83.97%   +0.60%     
==========================================
  Files          18       18              
  Lines        1149     1161      +12     
==========================================
+ Hits          958      975      +17     
+ Misses        191      186       -5     
Impacted Files Coverage Δ
src/access.rs 86.02% <33.33%> (-7.81%) ⬇️
src/lib.rs 87.56% <0.00%> (+0.51%) ⬆️
src/strategy/hybrid.rs 93.84% <0.00%> (+6.15%) ⬆️
src/debt/helping.rs 89.06% <0.00%> (+10.93%) ⬆️

@steffahn
Copy link
Contributor Author

steffahn commented Jul 5, 2022

@vorner I’ve addressed the unrelated CI failures (I don’t know if that’s the "proper" way to do this with the dependencies for 1.45 compilation, but it seems like it works); feel free to re-run CI.

Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

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

Thanks for the provided fixes. There's also some miri, but I think that one should be dealt with separately… it's a new miri's capability that wasn't there before and needs some more investigation.

Seems like it indeed helps and it can be compiled on the ancient versions. If you also could adjust the documentation, stating that the wrapper is not necessary any more in many cases (are there any cases left where it is necessary? It needs to be left in place for backwards compatibility, but maybe if there's no use case for it any more, it could be deprecated and hidden from documentation).

}
}

// Should probably be moved to a more appropriate place / perhaps more extensively tested / etc
Copy link
Owner

Choose a reason for hiding this comment

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

The more appropriate place is just the end of the file (not the middle), otherwise it is fine :-)

@steffahn
Copy link
Contributor Author

steffahn commented Jul 6, 2022

are there any cases left where it is necessary?

as already noted over in #77:

the actual AccessConvert is somewhat more general, as it allows – say – Box<dyn Foo> for a trait Foo: DynAccess, too.

Similarly, you can use it for things like Box<dyn DynAccess + Unpin> or Box<dyn DynAccess + Sync> or other more rare variantions on the trait object type that I didn't cover with any explicit implementation. (I chose the same selection three of auto-trait combinations (none, only send, and send+sync) that e. g. dyn Any in the standard library supports for downcasting; if there are others with practical use, they can be added later.)

@vorner
Copy link
Owner

vorner commented Jul 10, 2022

Hello again. Just wanted to check, are you still working on the docs, or did that one get lost in the rest of the discussion? 😇 Just that we both don't wait on something from the other side.

@steffahn
Copy link
Contributor Author

Thanks for the ping. I didn’t misunderstand, but I haven’t written the docs yet either; I’ll try to allocate some time to this soon :-)

@vorner
Copy link
Owner

vorner commented Dec 24, 2022

Hello.

I've discovered this still sits here. Not that I would have too much free time myself, but should I just take it over and write the docs, so it moves forward?

@steffahn
Copy link
Contributor Author

I forgot this PR even exists, so ehm... sure, feel free to take over, thank you.

vorner added a commit that referenced this pull request Dec 25, 2022
Clean up & document the changes from previous commit from #78.
@vorner
Copy link
Owner

vorner commented Dec 25, 2022

I've taken the main commit, put it onto current (which already had the CI issues mostly solved) master and made some docs for it and merged in #85.

I'll try to collect few more outstanding PRs and release a new version soon. If I don't get around to it by the end of the year, feel free to ping me.

Thank you for discovering the way to do this :-)

@vorner vorner closed this Dec 25, 2022
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.

None yet

3 participants