Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make Phobos compilable with preview=fieldwise #7624

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Make Phobos compilable with preview=fieldwise #7624

merged 2 commits into from
Oct 21, 2020

Conversation

nordlow
Copy link
Contributor

@nordlow nordlow commented Sep 10, 2020

Reopened to see if things build.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @nordlow! 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
✓ 21231 normal Unreachable warning for empty struct in VariantN with preview=fieldwise

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 + phobos#7624"

@nordlow
Copy link
Contributor Author

nordlow commented Sep 10, 2020 •
edited
Loading

Alternatively, is it possible to statically check whether an ==-expression constant folds or not?

Asked at Slack.

@nordlow
Copy link
Contributor Author

nordlow commented Sep 10, 2020 •
edited
Loading

Now correctly handles user-defined opEquals aswell, @MoonlightSentinel.

@MoonlightSentinel
Copy link
Contributor

Nice, that seems to work with and without the flag. But I'm not sure if we should enable it by default for all CI machines given that code could break when not compiling with -preview=fieldwise.

Let's merge the bugfix first and deal with the transition in the dmd pr when it becomes the default.

@nordlow
Copy link
Contributor Author

nordlow commented Sep 11, 2020 •
edited
Loading

Do you mean that this PR is the bugfix or you should I remove anything from this PR? It's ok for me to merge this.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Sep 11, 2020 •
edited
Loading

I meant reverting the changes to the Makefiles, we want to ensure Phobos is compilable without -preview=fieldwise.

andralex
andralex reviewed Sep 11, 2020
View reviewed changes
std/variant.d Outdated Show resolved Hide resolved
@nordlow
Copy link
Contributor Author

nordlow commented Sep 13, 2020

Nice, that seems to work with and without the flag. But I'm not sure if we should enable it by default for all CI machines given that code could break when not compiling with -preview=fieldwise.

Shall we revert the additions of -preview=dtorfields aswell?

@wilzbach
Copy link
Member

Shall we revert the additions of -preview=dtorfields aswell?

No that's fine and very unlikely to break something. Furthermore, that one is more of a bug fix.

@nordlow
Copy link
Contributor Author

nordlow commented Sep 13, 2020 •
edited
Loading

Corrected according to all comments. I'm done.

@nordlow nordlow changed the title Fieldwise revived Make Phobos compilable with preview=fieldwise Sep 13, 2020
@nordlow
Copy link
Contributor Author

nordlow commented Sep 20, 2020

Ping. Ready to roll?

@nordlow
Copy link
Contributor Author

nordlow commented Sep 28, 2020 •
edited
Loading

Ping again.

BTW: AFAICT, opPostMove will be possible if -preview=fieldwise becomes the default.

@MoonlightSentinel @nordlow
Fix 21231 - Unreachable warning for empty struct in VariantN...
c7e49f5 
... with preview=fieldwise.

Equality checks will never fail for instances of empty structs, hence
don't generate code checking `<` and `>` for them.

This was exposed by `-preview=fieldwise` which (as a side effect)
enables dmd to const-fold the aformentioned equality test in the
frontend.
@MoonlightSentinel @nordlow
Make Phobos compilable with preview=fieldwise
e824871
@nordlow
Copy link
Contributor Author

nordlow commented Oct 16, 2020

Reverted changes to win32.mak and win64.mak. Ready for merge?

@dlang-bot dlang-bot merged commit 54fade5 into dlang:master Oct 21, 2020
@nordlow nordlow deleted the fieldwise-revived branch October 19, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andralex andralex andralex left review comments

@thewilsonator thewilsonator thewilsonator approved these changes

@CyberShadow CyberShadow Awaiting requested review from CyberShadow

@MartinNowak MartinNowak Awaiting requested review from MartinNowak

@wilzbach wilzbach Awaiting requested review from wilzbach

Assignees
No one assigned
Labels
Merge:auto-merge Severity:Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nordlow @dlang-bot @MoonlightSentinel @wilzbach @andralex @thewilsonator