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.

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

Fix Issue 13392 and 11294 - class + alias this + cast(void*) == overzealous cast #9017

Merged
merged 1 commit into from Nov 29, 2018

Conversation

RazvanN7
Copy link
Contributor

Again op_overload mixes semantics. Before this patch when op_overload was called for a cast expression,
the result could mean 2 things: either an opCast exists or there is an alias this to be used for casting. 2 different semantics squashed in the same function call.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
11294 normal Object destruction with alias this
13392 normal class + alias this + cast(void*) == overzealous cast

Testing this PR locally

If 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 + dmd#9017"

@thewilsonator
Copy link
Contributor

Running ../dmd/generated/linux/release/64/dmd...
--
  | Serializing composite type BuildRequirements which has no serializable fields
  | Serializing composite type BuildOptions which has no serializable fields
  | source/dub/generators/generator.d(399): Error: template `std.typecons.BitFlags!(BuildOption, cast(Flag)false).BitFlags.opCast` does not match any template declaration

@RazvanN7
Copy link
Contributor Author

@thewilsonator
Copy link
Contributor

You'll need to give this a kick to restart the doc tester.

@RazvanN7
Copy link
Contributor Author

Is it failing spuriously? I can't imagine how my patch would affect the documentation tester.

@RazvanN7
Copy link
Contributor Author

Buildkite is failing. I have to investigate.

@RazvanN7
Copy link
Contributor Author

dls looks like is using an older version of dub

@thewilsonator
Copy link
Contributor

Any idea why iz and mir-algorithm segfault?

@RazvanN7
Copy link
Contributor Author

@thewilsonator I fixed those. It was an implementation problem. dls is still failing because it is not using the latest version of dub.

@wilzbach Anything we can do about this, except from making a PR to change the dub version in dls?

@thewilsonator
Copy link
Contributor

Thanks.

@RazvanN7
Copy link
Contributor Author

I think we should wait until we fix dls before merging this, otherwise we will end up failing buildkite for the entire PR queue.

@thewilsonator
Copy link
Contributor

dls has a dub.selections.json committed to the repo (which I think is usually placed under a .gitignore but w/e) but it is using the latest tag of dub. Do we need to do another tag of dub?

@PetarKirov
Copy link
Member

PetarKirov commented Nov 28, 2018

@s-ludwig @MartinNowak @wilzbach what do you think about allowing out-of-band patch releases for dub? What I propose is to allow tagging new patch releases, so that projects using dub as a library could receive bug fixes without waiting for a new dmd release.
In this case, we need a new patch release including dlang/dub#1600, in order to make https://github.com/d-language-server/dls compatible with this dmd PR.

The process should be as simple and fast as pushing a new tag to the dub repo. I don't think anything else would be needed.

@s-ludwig
Copy link
Member

No real objection from my side, I've even done out-of-band patch releases a few times in the past. However, the change log process has changed recently, which may be a problem. The version file also needs to be updated in addition to the tag.

@wilzbach
Copy link
Member

wilzbach commented Nov 28, 2018

@s-ludwig @MartinNowak @wilzbach what do you think about allowing out-of-band patch releases for dub? What I propose is to allow tagging new patch releases, so that projects using dub as a library could receive bug fixes without waiting for a new dmd release.
In this case, we need a new patch release including dlang/dub#1600, in order to make https://github.com/d-language-server/dls compatible with this dmd PR.

The process should be as simple and fast as pushing a new tag to the dub repo.

Sure, we have done so before, but only as e.g. 1.12.0-1 as then @MartinNowak's release scripts don't need adaption.
Also, it needs to be done from stable and the PR still needs to be cherry-picked to it as the PR was targetting master.

I don't think anything else would be needed.

If DLS has a dub.selections.json, that one might need to be updated and a new version of DLS would need to be tagged too.

@wilzbach
Copy link
Member

Also, it needs to be done from stable and the PR still needs to be cherry-picked to it as the PR was targetting master.

-> dlang/dub#1601

@jacob-carlborg
Copy link
Contributor

dls has a dub.selections.json committed to the repo (which I think is usually placed under a .gitignore but w/e) but it is using the latest tag of dub. Do we need to do another tag of dub?

@thewilsonator dub.selections.json should be committed for applications but ignored for libraries.

@PetarKirov
Copy link
Member

PetarKirov commented Nov 28, 2018

If DLS has a dub.selections.json, that one might need to be updated and a new version of DLS would need to be tagged too.

Yeah, see: d-language-server/dls#17.

Also, it needs to be done from stable and the PR still needs to be cherry-picked to it as the PR was targetting master.

-> dlang/dub#1601

Thanks!

@wilzbach
Copy link
Member

Restarted the dls build as a new version which selects dub v1.12.1-alpha.1 should be up now.

@RazvanN7
Copy link
Contributor Author

All green! @thewilsonator

@dlang-bot dlang-bot merged commit 42c7101 into dlang:master Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants