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

std.typecons: Greatly simplify scoped() #8460

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kinke
Copy link
Contributor

@kinke kinke commented May 13, 2022

The previous implementation was just terrible, presumably dating back to when align(N) was buggy.

@kinke kinke requested a review from MetaLang as a code owner May 13, 2022 14:41
@dlang-bot
Copy link
Contributor

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#8460"

@kinke
Copy link
Contributor Author

kinke commented May 13, 2022

FYI @ljmf00.

Copy link
Member

@ljmf00 ljmf00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

std/typecons.d Show resolved Hide resolved
@ljmf00
Copy link
Member

ljmf00 commented May 13, 2022

Ahhhhhgg, we are having the same problem as the TLS alignment on druntime.

@kinke
Copy link
Contributor Author

kinke commented May 13, 2022

Yeah, looks like there's a corresponding DMD backend issue for 32-bit COFF too. :(

@kinke
Copy link
Contributor Author

kinke commented Jun 20, 2022

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well, pretty nice cleanup!

@PetarKirov
Copy link
Member

@kinke Windows CI tests are failing:

object.Error@(0): Access Violation
----------------
0x02A3E8ED in rt_finalize2
0x019AF400 in object.destroy!(false, std.typecons.__unittest_L8826_C9.C1long).destroy at D:\a\1\dmd\druntime\import\object.d(4154)
0x019C8577 in std.typecons.__unittest_L8826_C9.alignmentTest at D:\a\1\phobos\std\typecons.d(8856)
0x019C8526 in std.typecons.__unittest_L8826_C9 at D:\a\1\phobos\std\typecons.d(8874)
0x02A4FCB3 in int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*))
0x02A4D0A6 in _d_run_main2
0x02A37234 in _d_run_main
0x00FBEECC in unittest._d_cmain!().main at D:\a\1\dmd\druntime\import\core\internal\entrypoint.d(30)
0x771105C9 in BaseThreadInitThunk
0x776E784D in RtlGetAppContainerNamedObjectPath
0x776E781D in RtlGetAppContainerNamedObjectPath
1/133 modules FAILED unittests

@kinke
Copy link
Contributor Author

kinke commented Mar 27, 2023

Yeah, same issue as ~a year ago. I assumed the compiler fix for https://issues.dlang.org/show_bug.cgi?id=16098 finally fixed this for DMD, but no it didn't.

@thewilsonator
Copy link
Contributor

****** FAIL release32 std.typecons
core.exception.AssertError@../dmd/druntime/import\core\lifetime.d(207): chunk is not aligned.

@LightBender LightBender added the Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
@LightBender
Copy link
Contributor

@kinke, what would it take to unblock this? I have Windows but not enough knowledge to reproduce on my own.

@kinke
Copy link
Contributor Author

kinke commented Oct 27, 2024

It presumably still takes a DMD backend fix wrt. violated (over-)alignment; maybe only 32-bit codegen specific, maybe not. Maybe for stack allocations, maybe for static data.

@thewilsonator
Copy link
Contributor

Looking good so far. (Ignore the Buildkite style failure, that is unrelated and has been fixed), the MacOS failures are random and expected.

@thewilsonator
Copy link
Contributor

oh, still fails on windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Refactoring Salvage This PR needs work, but we want to keep it and it can be salvaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants