Skip to content

Fix Issue 22717 - TypeInfo_Struct.equals swaps lhs and rhs parameters#13593

Merged
dlang-bot merged 4 commits into
masterfrom
fix22717
Feb 2, 2022
Merged

Fix Issue 22717 - TypeInfo_Struct.equals swaps lhs and rhs parameters#13593
dlang-bot merged 4 commits into
masterfrom
fix22717

Conversation

@kinke

@kinke kinke commented Jan 31, 2022

Copy link
Copy Markdown
Contributor

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

dlang-bot commented Jan 31, 2022

Copy link
Copy Markdown

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"

Comment thread src/dmd/clone.d Outdated
Leading to a swapped `rhs.opEquals(lhs)` check *if* the opEquals
method took its parameter by ref.
Comment thread src/build.d
@kinke

kinke commented Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

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

kinke commented Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

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

kinke commented Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

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
@kinke kinke marked this pull request as ready for review February 1, 2022 16:11
@kinke

kinke commented Feb 2, 2022

Copy link
Copy Markdown
Contributor Author

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

ibuclaw commented Feb 2, 2022

Copy link
Copy Markdown
Member

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. ;-)

@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.

3 participants