Skip to content

Conversation

@kinke
Copy link
Contributor

@kinke kinke commented Jun 27, 2020

The previous test really depended on -preview=fieldwise.
See dlang/druntime#3142 (comment).

The previous test really depended on `-preview=fieldwise`.
See dlang/druntime#3142 (comment).
@CyberShadow
Copy link
Owner

Hi,

Sorry, I'm having some trouble understanding this change. The linked pull request, as well as the pull request linked from it, doesn't describe or rationalize any change in behaviour.

As far as I can see there is some problem with the union, but even from looking at the -preview=fieldwise documentation (which, as far as I can tell, is documented only in the 2.085.0 changelog), I can't infer how this leads to an intentional breakage of comparing structs containing unions. The test definitely would never have passed if the compiler was using memcmp for comparison, either on Transformation or even on Transformation.Replace.

@kinke
Copy link
Contributor Author

kinke commented Jun 27, 2020

The compiler does use a memcmp/is for that struct without -preview=fieldwise, as clearly shown by my linked example. It just did a field-wise comparison when the array comparison was lowered to a __equals() call, which is the case for this test, as you use an array literal (typed as a dynamic array). If you used a static array, as shown in my example, it (correctly) wouldn't work, because then the compiler does the array equality comparison itself (memcmp), without that __equals() in druntime with diverging semantics (rectified in that PR).

@CyberShadow
Copy link
Owner

Hmm, I see. Thanks for clarifying. But, as this silently changes language behaviour, this doesn't seem to be a good way to effect such changes. Is it possible to make such comparisons cause a deprecation warning, forcing users to explicitly disambiguate into either a bitwise (is) comparison or a field-wise (.tupfeof) comparison?

I feel like silent language changes like this should be approved by BDFLs before being accepted.

@kinke
Copy link
Contributor Author

kinke commented Jun 27, 2020

This came unexpected to me too, so I don't know how to best handle that. I think -preview=fieldwise should probably be promoted to the default soon (e.g., druntime is compiled with it, Phobos isn't...), as that's probably what everybody expects to happen anyway.

@kinke
Copy link
Contributor Author

kinke commented Jun 27, 2020

Reduced it to this: https://run.dlang.io/is/OJDRNK

This hasn't worked before v2.078, that's when this silent change was introduced.

@CyberShadow
Copy link
Owner

Applied as 994f4ac, thanks.

@kinke kinke deleted the patch-1 branch June 27, 2020 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants