phobos icon indicating copy to clipboard operation
phobos copied to clipboard

Published 20 hours ago • verified publisher icon dlang

Make `toDelegate` safe for function pointers

Open Bolpat opened this issue 1 year ago • 10 comments

Bolpat avatar Jun 20 '23 15:06 Bolpat

Thanks for your pull request and interest in making D better, @Bolpat! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 18171 enhancement std.functional.toDelegate should be usable in @safe

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8769"

dlang-bot avatar Jun 20 '23 15:06 dlang-bot

Why is there a new branch for function pointers, is the final else case obsolete now?

Because toDelegate works with any callable, the else branch cannot be @safe in general. To be @safe for function pointers, toDelegate needed a specific branch (or it looked like the best solution to me).

Bolpat avatar Jun 21 '23 08:06 Bolpat

  • $(LI Does not work with @safe functions.)

This line is no longer accurate now

That should have been a review comment.

Bolpat avatar Jun 21 '23 08:06 Bolpat

That should have been a review comment.

I can't comment on code not in the diff

dkorpel avatar Jun 21 '23 09:06 dkorpel

Because toDelegate works with any callable

There's now branches for delegate, function, and opCall, what remaining cases does toDelegate support?

dkorpel avatar Jun 21 '23 09:06 dkorpel

Ping @Bolpat

dkorpel avatar Jun 28 '23 09:06 dkorpel

Ping @Bolpat

dkorpel avatar Jul 05 '23 11:07 dkorpel

Sorry, I was absent for a while. I looked into the failed checks and didn’t understand what’s wrong.

Should I force-push?

Bolpat avatar Oct 20 '23 10:10 Bolpat

@Bolpat yes, please rebase & force-push.

PetarKirov avatar Oct 31 '23 11:10 PetarKirov

Also, please answer https://github.com/dlang/phobos/pull/8769#issuecomment-1600513792

dkorpel avatar Oct 31 '23 14:10 dkorpel