Start deprecation period of identical functions in a single module #8429
Conversation
|
Thanks for your pull request, @wilzbach! Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#8429" |
wilzbach
force-pushed
the
duplicate-declaration
branch
from
d52e5be to
d9ec356
Compare
|
There's the issue of: This is a common pattern in C, should it be allowed in D? |
wilzbach
force-pushed
the
duplicate-declaration
branch
from
d9ec356 to
47e756e
Compare
I don't see a reason to forbid it. Note that this PR doesn't touch this part (see the tests for examples), it will only trigger a deprecation warning when the mangling of two function is equivalent AND both have a body. |
|
It's been previously stated that when D interfaces with another language it should follow it's own overloading and lookup rules. Otherwise we would need implement argument dependent lookup for One use case I could see would be to add overloads so that the D binding allows passing the same values as the C function would allow without requiring casts. For example: extern (C) void foo(bool);
extern (C) void foo(int);
foo(true);
foo(1); |
|
Another use case is, not sure if you're changes cover that: template Metaclass
{
extern (Objective-C) interface Class
{
extern (C) pragma(mangle, "objc_lookUpClass") static typeof(this) objc_lookUpClass(const(char)* name);
}
}
extern (Objective-C) NSObject
{
mixin Metaclass;
}
extern (Objective-C) NSObject
{
mixin Metaclass;
}Otherwise calling |
|
@jacob-carlborg having duplicate declarations is still completely legal (a previous version of this push tried to deprecate this too, but druntime abused this pattern too).
Yep that shouldn't be affected by this change, but note that if you meant to add "class" there, having two duplicate classes of the same name was already restricted before this PR: https://run.dlang.io/is/74BDoy |
Aha, ok. Then I'm fine. |
|
I'm a bit unclear on the process, when do we add a changelog entry? |
wilzbach
force-pushed
the
duplicate-declaration
branch
from
47e756e to
85cc64a
Compare
Sorry. I expected more outcry / request for changes on this one. Added the changelog entry now. |
|
Is Jenkins disabled? DWT might be affected. But since it’s only a deprecation I think it shouldn’t be a problem. |
wilzbach
added
the
Merge:72h no objection -> merge
wilzbach
force-pushed
the
duplicate-declaration
branch
4 times, most recently
from
d4e3895 to
80e3ce2
Compare
Yep it's running again, but here's what I locally get: I will have a look into the segfault, but the deprecations messages are legit as C doesn't mangle its parameters. Try an |
|
I think there are several functions that have no reason to be |
JinShil
removed
the
Merge:72h no objection -> merge
|
Removed the "72h no object -> merge" label as it was applied 7 days ago and this isn't passing the auto tester or Jenkins. I deprecated the auto tester failing test, so let's see if it passes. |
|
I'm itching to merge this, but what's going on with Jenkins right now? |
|
Failure is definitely related, as @wilzbach mentioned above: |
|
Should I wait with updating DWT? |
|
No feel free to update DWT. Once I have time for this, I will manage to reproduce the segfault from DWT, reduce it and it to the testsuite anyhow. |
|
Is there another PR for updating the spec? |
|
Thanks! |
MoonlightSentinel
force-pushed
the
duplicate-declaration
branch
from
56cefca to
a8aa7f9
Compare
|
Also not really happy about this deprecation, but if it allows projects a more pleasant transition ... Should we already define a release for the end of the deprecation and mention it in the error message? (e.g. |
test/fail_compilation/fail2789.d
Outdated
| fail_compilation/fail2789.d(67): Error: function `fail2789.f_ExternC1()` cannot be overloaded with another `extern(C)` function at fail_compilation/fail2789.d(66) | ||
| fail_compilation/fail2789.d(70): Error: function `fail2789.f_ExternC2(int)` cannot be overloaded with another `extern(C)` function at fail_compilation/fail2789.d(69) | ||
| fail_compilation/fail2789.d(73): Error: function `fail2789.f_ExternC3()` cannot be overloaded with another `extern(C)` function at fail_compilation/fail2789.d(72) | ||
| fail_compilation/fail2789.d(67): Error: function `fail2789.f_ExternC1()` conflicts with previous declaration at fail_compilation\fail2789.d(66) |
There was a problem hiding this comment.
Side note, why isn't the file name / line highlighted ?
There was a problem hiding this comment.
Ping @MoonlightSentinel . And why the backslash ?
There was a problem hiding this comment.
Because Locs are usually not highlighted in error messages (but we could change that in another PR).
Replaced the backslash
| struct S | ||
| { | ||
| static void foo(int) {} | ||
| static void foo(double) {} |
There was a problem hiding this comment.
Please remind me why this shouldn't be allowed? These two symbols are part of a struct and therefore will get mangled as D symbols, even though the linkage is extern (C).
The changelog entry says: "Note that this change is only relevant for mangling schemes that do no support
overloading". The D mangling scheme does support overloading. Obviously the implementation is not looking at the mangled name when deciding if it should issue a deprecation message or not. Make up your minds.
There was a problem hiding this comment.
As discussed above, the current behaviour of extern(C) not changing the mangling inside of a struct is a bug because the function still adheres to the C ABI.
The current implementation could cater for that special case but I don't think that it should. Having the deprecation now will allow for a smoother transition when fixing extern(C) inside structs.
|
@Geod24 Rewrote the changelog and improved the deprecation message. |
They need to have different parameter lists/storage classes.
MoonlightSentinel
force-pushed
the
duplicate-declaration
branch
from
8118769 to
ca701de
Compare
Geod24
added
Merge:auto-merge
Merge:auto-merge-squash
and removed
Merge:auto-merge
labels
|
This pull destroyed compiler diagnostic for conflicts in overload sets created by aliases. before: now: |
|
When compiling Visual D with 2.095-beta1 I get a lot of deprecation messages due to this change:
Is this expected even for non-static member functions? |
I have the same problem with some of my projects. The reason for the change was to remove the mangling of But then everyone changed their mind when it comes to remove the actual mangling: #11786. I don't know what's going on here, it's a mess. |
The code is probably just looking at the linkage and not the actual mangling. |
|
I'll have a look |
|
I'm fine with the change for functions with identical mangling. So the change is good for free functions, not so sure about static member functions (with changed mangling), but non-static member functions are not a thing in C, so don't have to obey the same ABI anyway. |
The false positive hides the actual error. Fix #12053
Fix to accept the name mangling for (static) member funtions: #12054 |
…lang#8429) Start deprecation period of identical functions in a single module merged-on-behalf-of: unknown
|
This PR caused a regression: https://issues.dlang.org/show_bug.cgi?id=22975 |
Initially started in #7577, but reverted in #7969 due to the immediate 2.079 release and a reported issue for it (https://issues.dlang.org/show_bug.cgi?id=18385).
This now officially starts the deprecation period of identical functions in a single module.
The examples provided in 18385 are:
They clearly can't work because the mangling of these functions is the same.
The deprecation period is ten releases after the new deprecation DIP.
Note that the following will still work:
or:
See also: #8428