Fix Issue 23966 - [REG2.102] Cannot use traits(getAttributes) with overloaded template #15353
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
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 "stable + dmd#15353" |
compiler/test/compilable/cppmangle.d
Outdated
| @@ -528,7 +528,7 @@ version (CppMangle_Itanium) | |||
| static assert(basic_ostream!(char, char_traits!char).food.mangleof == "_ZNSo4foodEv"); | |||
| static assert(basic_iostream!(char, char_traits!char).fooe.mangleof == "_ZNSd4fooeEv"); | |||
|
|
|||
| static assert(func_18957_2.mangleof == `_Z12func_18957_2PSaIiE`); | |||
| //static assert(func_18957_2.mangleof == `_Z12func_18957_2PSaIiE`); | |||
There was a problem hiding this comment.
func_18957_2 is an overload set with a normal function and a function template. Up until now dmd just chose the function because of the bug in isUnique. I would argue that this should be an error. Thoughts?
There was a problem hiding this comment.
Perhaps put this comment in the test, so it's clear why the assert is disabled
There was a problem hiding this comment.
The comment was used as a discussion starter. I don't expect this PR will be merged with this test commented.
There was a problem hiding this comment.
One way to fix this without introducing any breaking changes is to add an optional bool parameter to isUnique which is set only when isUnique is called from semanticTiargs. That way, we can preserve the existing behavior with respect to the mangling of functions. Although, I still tend to argue that the way this patch deals with things is the more correct approach.
There was a problem hiding this comment.
I'm not terribly concerned with changing the name mangling. D libraries compiled with one version of DMD and linking with modules compiled with another version are a risky business regardless.
RazvanN7
added
the
Review:Industry
compiler/test/compilable/cppmangle.d
Outdated
| @@ -528,7 +528,7 @@ version (CppMangle_Itanium) | |||
| static assert(basic_ostream!(char, char_traits!char).food.mangleof == "_ZNSo4foodEv"); | |||
| static assert(basic_iostream!(char, char_traits!char).fooe.mangleof == "_ZNSd4fooeEv"); | |||
|
|
|||
| static assert(func_18957_2.mangleof == `_Z12func_18957_2PSaIiE`); | |||
| //static assert(func_18957_2.mangleof == `_Z12func_18957_2PSaIiE`); | |||
There was a problem hiding this comment.
Perhaps put this comment in the test, so it's clear why the assert is disabled
|
@RazvanN7 can you please make a posting to the n.g. about this fix, so people who it was failing for can see it? |
…erloaded template
…erloaded template (dlang#15353)
…erloaded template (dlang#15353)
FuncDeclaration.isUniquedid not take into account template overloads. This caused some FuncAliasDeclarations (used bytraits(getOverloads)) to be wrongfully substituted and therefore tripped the deprecation. I fixed theisUniquefunction to take into consideration templates, however, this affects the mangling of some functions (see commented test).