Skip to content

Conversation

@kinke
Copy link
Contributor

@kinke kinke commented Jan 31, 2022

Leading to a swapped rhs.opEquals(lhs) check if the opEquals method took its parameter by ref.

This depends on dlang/druntime#3718.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 31, 2022

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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

Auto-close Bugzilla Severity Description
22717 critical object.TypeInfo_Struct.equals swaps lhs and rhs parameters

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13593"

Leading to a swapped `rhs.opEquals(lhs)` check *if* the opEquals
method took its parameter by ref.
@kinke
Copy link
Contributor Author

kinke commented Jan 31, 2022

auto-tester failing is to be expected, as it doesn't support cloning same-named druntime/Phobos branches for PRs originating from the official repo. AFAIK, Azure and Cirrus already did support that; CircleCI newly does with an extra commit here. Buildkite needs something like dlang/ci#452; only LDC should fail.

kinke added a commit to kinke/ldc that referenced this pull request Feb 1, 2022
…g opEquals order

Affecting the frontend itself, see
https://issues.dlang.org/show_bug.cgi?id=22717.

Preparing LDC early is required to make DMD's Buildkite CI pass with
the proposed fix in dlang/dmd#13593.
@kinke
Copy link
Contributor Author

kinke commented Feb 1, 2022

I've changed the fix to only apply if the compiler is going to define __VERSION__ as 2099 - currently depending on whether there's a .git subdir in the DMD repo at build-time: if there is, git describe currently returns some v2.098.0-…, and __VERSION__ is going to be 2098; otherwise it's 2099 (from the fallback VERSION file).

@kinke
Copy link
Contributor Author

kinke commented Feb 1, 2022

Buildkite still needs to bump its LDC ref once it's prepared for the upcoming breaking change (ldc-developers/ldc#3910), then this PR is ready.

kinke added a commit to ldc-developers/ldc that referenced this pull request Feb 1, 2022
…g opEquals order (#3910)

Affecting the frontend itself, see
https://issues.dlang.org/show_bug.cgi?id=22717.

Preparing LDC early is required to make DMD's Buildkite CI pass with
the proposed fix in dlang/dmd#13593.
kinke added a commit to dlang/ci that referenced this pull request Feb 1, 2022
dlang-bot pushed a commit to dlang/ci that referenced this pull request Feb 1, 2022
@kinke kinke marked this pull request as ready for review February 1, 2022 16:11
@kinke
Copy link
Contributor Author

kinke commented Feb 2, 2022

Ready now.

Note: all compilers with __VERSION__ >= 2099 are going to 'miscompile' all older DMD frontends, due to the unfortunate TemplateInstanceBox.opEquals() const-hack and according order-dependency.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 2, 2022

Well we never claimed that newer versions could build older - and, usually it's deprecations and import bugfixes that mean we can't go back more than a couple anyway without having to downgrade the host compiler. ;-)

@dlang-bot dlang-bot merged commit adcc8b0 into master Feb 2, 2022
@kinke kinke deleted the fix22717 branch February 2, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants