-
-
Notifications
You must be signed in to change notification settings - Fork 741
fix Issue 13780 - Empty ParameterIdentifierTuple for function literal #3620
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
Conversation
I'm not a fan of this, since I'm hoping to get rid of parameter names from function and delegate types. |
Why would we want to do that? Are the parameter names part of the type or something? |
In the future, please add a link to the issue from your PR description and vice versa, and include the issue title in the PR title and commit message. |
I think there was a copy paste error when I copied the issue. It was supposed to be Issue 13780 - Empty ParameterIdentifierTuple for function literal (https://issues.dlang.org/show_bug.cgi?id=13780) and not the possibly related but not-fixed-by-this Issue 14947 - std.traits: ParameterIdentifierTuple on an 'interface' not working (https://issues.dlang.org/show_bug.cgi?id=14947). The branch name is also affected, although I don't know what impact that will have in practice? (misleading git history?) |
Branch names are local to your repository, so don't matter. GitHub seems to even encourage you to delete your branch after a PR is merged. |
@CyberShadow yes, but the merge message says something like "Merge pull request #1234 from user/branch" |
I don't think anyone looks at that, besides, the PR title is included in the commit message too. |
I wouldn't worry about it, that happens all the time. |
d176bca
to
336d8ac
Compare
OK, good. I changed the commit message, though. |
336d8ac
to
3995f6a
Compare
ParameterIdentifierTuple intentionally did not accept function pointers and delegates. This surprised people, particularly when using function literals, as indicated in issue 13780. This commit changes that, allowing the issue to be closed.
3995f6a
to
5b8b90e
Compare
@"get rid of parameter names from function and delegate types." I presented one such motivating example here: |
FWIW, this PR is missing tests. |
What's the status of this? |
There are unittests already, kindly detail what more is expected with regards to "needs work"? |
My bad, I see now that this changes existing unit tests instead of adding new ones. @yebblies Do you object to this pull being merged? |
I don't think parameter names should be part of the function type, so yes. |
I'd like to get some kind of resolution on this. Ping @WalterBright @andralex? |
I agree with @yebblies, names shouldn't be part of the function type (!). This is not only speaking as a compiler dev, but also as somebody who has been bitten by the (mostly necessary) inconsistencies in this "feature" when writing meta-magical code before. |
I'm not really clear on how it works in the compiler. Why is it that the parameter names of a regular function are not part of the type, but they are for lambdas? In other words, why is this a problem: pragma(msg, ParameterIdentifierTuple!((int n) => 0)); //Prints 'tuple()' But this isn't: int someFun(int n)
{
return 0;
}
pragma(msg, ParameterIdentifierTuple!someFun); //Prints 'tuple("n")' |
As far as I can see, the fact that parameter names ever leaked into the type in the first place is due to an implementation quirk. I don't know of any reasoning that would have gone into allowing that. The main issue is that parameter names of course aren't proper parts of the type system. Two functions with identical signature but different parameter names have the same type. Parameter names aren't part of the mangle string. Thus, you can run into inconsistencies where two types are seemingly different (e.g. looking at Since parameter names aren't part of the type (as in, relevant to the type system), the only way to resolve these inconsistencies without relying on DMD implementation internals seems to be to make sure they don't ever leak into the type in the first place. |
So are you saying that ParameterIdentifierTuple should not work for any function - lambda, named or otherwise? |
Not quite – the template could still work when given a direct alias to a function/lambda/… (albeit maybe with a different syntax for |
This has been open for 2 years without being merged, so I'm going to close it. |
ParameterIdentifierTuple intentionally did not accept function pointers and delegates. This surprised people, particularly when using function literals, as indicated in issue 13780. This commit changes that, allowing the issue to be closed.
https://issues.dlang.org/show_bug.cgi?id=13780