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 toDelegate safe for function pointers #10599

Merged

Conversation

0xEAB
Copy link
Member

@0xEAB 0xEAB commented Dec 29, 2024

Revamp of #8769.

While I’m not sure whether it covers all use-cases that @Bolpat’s patch had considered and that exist (also see Dennis’ unanswered question over there), it at least passes the testsuite on my machine.

As this is a @trusted hack, I’d appreciate my patch to be thoroughly reviewed.

@0xEAB 0xEAB requested a review from thewilsonator December 29, 2024 05:58
@0xEAB 0xEAB requested a review from andralex as a code owner December 29, 2024 05:58
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @0xEAB!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#10599"

@0xEAB 0xEAB force-pushed the bolpat/patch-1/but-it-actually-works branch 2 times, most recently from c569c33 to 89d37f4 Compare December 29, 2024 07:25
std/functional.d Show resolved Hide resolved
std/functional.d Outdated Show resolved Hide resolved
std/functional.d Outdated Show resolved Hide resolved
@0xEAB 0xEAB force-pushed the bolpat/patch-1/but-it-actually-works branch from 5ebfe85 to ac35640 Compare December 30, 2024 06:55
@thewilsonator thewilsonator merged commit 262645b into dlang:master Dec 30, 2024
9 checks passed
Comment on lines +1894 to +1895
static assert(!is(typeof(&s1.opCall) == delegate));
S1 s1;
Copy link
Contributor

Choose a reason for hiding this comment

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

What’s that doing?
In static assert(!is(typeof(&s1.opCall) == delegate));, the local variable s1 isn’t in scope.
If you swap the lines, the static assert passes.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, it’s a copy&paste of this code:

phobos/std/functional.d

Lines 1891 to 1901 in 8c6fca9

struct S1 { int opCall() { inc(myNum); return myNum; } }
static assert(!is(typeof(&s1.opCall) == delegate));
S1 s1;
auto getvals1 = toDelegate(s1);
assert(getvals1() == 4);
struct S2 { static int opCall() { return 123456; } }
static assert(!is(typeof(&S2.opCall) == delegate));
S2 s2;
auto getvals2 =&S2.opCall;
assert(getvals2() == 123456);

https://github.com/dlang/phobos/blame/8c6fca9ccdb30b69b627808c698526a76f7c4248/std/functional.d#L1891-L1901

0xEAB added a commit to 0xEAB/phobos that referenced this pull request Jan 24, 2025
0xEAB added a commit to 0xEAB/phobos that referenced this pull request Jan 24, 2025
0xEAB added a commit to 0xEAB/phobos that referenced this pull request Jan 24, 2025
thewilsonator pushed a commit that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants