Fix Issue 20539 - std.conv.to: internal overload conflict for enums with base types that have a catch-all opEquals overload #7378
Conversation
ghost
requested a review
from 
|
Thanks for your pull request and interest in making D better, @berni44! 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 fetch digger
dub run digger -- build "master + phobos#7378" |
std/conv.d
Outdated
| // needs to be outside of the unittest, because else a frame pointer issue | ||
| // makes it work unintentionally; see next unittest for the version with | ||
| // the struct being inside | ||
| struct Test20539 |
There was a problem hiding this comment.
This struct won‘t have a frame pointer if you declare it as static
There was a problem hiding this comment.
I'm not familiar enough with frame pointers yet. Anyway, this is the original test and it doesn't help to modify it, if that's what you want to do. I just realised that the test passed, when I put the struct inside the unittest. Having had a look, why this is, I found a "could not access frame pointer" problem in the overload using parse. So what I did was to add both versions as separate unittests and make sure they both work and do the same.
There was a problem hiding this comment.
There is no difference between the external or static declaration IIRC (except the visibility of course). This isn't required but this allows to condense bug tests inside one unittest and avoids version(StdUnittest).
Anyway, do as you see fit
There was a problem hiding this comment.
Ah, sorry, now I got it. I checked that indeed the bug is there, when I replace the global struct with the static one (for extra security, not because I do not believe you). This also helps to remove this stupid toHash. Thank you!
There was a problem hiding this comment.
[..] (for extra security, not because I do not believe you).[...]
Better safe than sorry
Thank you!
You're welcome
with base types that have a catch-all opEquals overload (?)
| } | ||
|
|
||
| // issue 20539 | ||
| @safe pure unittest |
There was a problem hiding this comment.
Is there a good reason to split this into 2 unittest instead of e.g. 2 scopes? That would avoid the duplicate import and unittest declaration
There was a problem hiding this comment.
Don't know. It feels a little bit like two different things. The first one guards against the bug, while the second one guards against not introducing a new bug with the bug fix. On the other hand, they are quite similar. Do you mind me leaving it, like it is?
There was a problem hiding this comment.
No, it's mostly a matter of preference (except maybe a negligible difference in compile/run time)
|
Closing in favor of: #7749 |
It wasn't easy to decide, which of the two overloads should be choosen: The first one uses the names of the enums, while the second one uses
opEquals. I decided on the second one, although the first one would have needed only one template constraint. The reason for this decission is that else the behaviour would differ, depending on the struct being inside the unittest or outside (due to a frame pointer issue).