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: let delegate_noop can handle constGeneric #679

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Decodetalkers
Copy link
Contributor

@Decodetalkers Decodetalkers commented Nov 29, 2023

the macro of delegate_noop cannot handle like
struct A<const X: unsize>, such kind of struct, so I want to fix it

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fdd88c4) 73.03% compared to head (2fca6dc) 73.03%.

❗ Current head 2fca6dc differs from pull request most recent head bd57467. Consider uploading reports for the commit bd57467 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #679   +/-   ##
=======================================
  Coverage   73.03%   73.03%           
=======================================
  Files          48       48           
  Lines        7841     7841           
=======================================
  Hits         5727     5727           
  Misses       2114     2114           
Flag Coverage Δ
main 58.32% <ø> (ø)
test-- 78.07% <ø> (ø)
test--server_system 61.07% <ø> (ø)
test-client_system- 68.90% <ø> (ø)
test-client_system-server_system 51.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elinorbgr
Copy link
Member

Thanks, the principle looks good, I'll take some time later to carefully review the macro.

More generally for this change, would you mind:

  • Add some text in the documentation of the macro explaining how to use it
  • Introduce the same change in wayland-server as well which has a similar macro
  • Add relevant entries to the changelog of the two crates

@Decodetalkers Decodetalkers force-pushed the constGeneric branch 2 times, most recently from 2a378db to 424f600 Compare November 29, 2023 14:28
@Decodetalkers
Copy link
Contributor Author

Thanks, the principle looks good, I'll take some time later to carefully review the macro.

More generally for this change, would you mind:

* Add some text in the documentation of the macro explaining how to use it

* Introduce the same change in `wayland-server` as well which has a similar macro

* Add relevant entries to the changelog of the two crates

Sure, I am happy to do this

@Decodetalkers Decodetalkers force-pushed the constGeneric branch 4 times, most recently from 8c2e1ae to 2fca6dc Compare November 30, 2023 02:38
@elinorbgr
Copy link
Member

Does the macro as it is here support types with both const-generic and regular generics? Something like App<const U: usize, T> ?

@Decodetalkers
Copy link
Contributor Author

I just let const before :, so the words after : will also work I think

@elinorbgr
Copy link
Member

I'm not sure about that, it seems to me that the pattern you used, $(@< $( const $lt:tt $( : $clt:tt $(+ $dlt:tt )* )? ),+ >)? would match a list like const U: usize, const V: usize, ..., but not a list mixing const arguments and regular type arguments?

@Decodetalkers
Copy link
Contributor Author

Ah.. I see, I fogot that generic types can be muti types..

@Decodetalkers Decodetalkers force-pushed the constGeneric branch 2 times, most recently from 78fdbfb to 910a8f7 Compare December 4, 2023 15:29
the macro of delegate_noop cannot handle like
struct A<const X: unsize>, such kind of struct, so I want to fix it
@Decodetalkers
Copy link
Contributor Author

Ok, now it is ok.. I try add muti generic type,

like

struct A<const X: usize, Y: Sized, Z> {
     it: Z
}

it seems ok

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.

2 participants