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 21038 - wchar and dchar string alignment should be 2 and 4,… #11528

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

WalterBright
Copy link
Member

… respectively

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
21038 major wchar and dchar string alignment should be 2 and 4, respectively

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

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Also this issue is marked major. Either this PR should target stable or the issue should be reprioritised.

test/runnable/mars1.d Outdated Show resolved Hide resolved
@WalterBright
Copy link
Member Author

This issue has been there for more than 10 years, and was just reported. So I'm not so sure it's major.

@Geod24
Copy link
Member

Geod24 commented Aug 7, 2020

I think the categorization is fair, any bad codegen bug should be major because of how hard they are to track down. Issues which are rejects-valid are annoying but trivial to spot. accepts-invalid is a notch up, but still at compile time. But bad codegen can be some of the worst bugs to deal with, as you've experienced over the past few days.

@WalterBright
Copy link
Member Author

Fixing some bugs can be disruptive, and does one really want regressions in stable?

But bad codegen can be some of the worst bugs to deal with, as you've experienced over the past few days.

I know all about finding codegen bugs. The original test suite was designed to minimize effort at tracking them down. But the current test suite has evolved into an edifice that makes it perversely difficult to track down problems. For example, the first program built by a new compiler shouldn't be a test harness that requires it to be perfect before it can even run. The compiler's internal bug checks (i.e. asserts) should not be bypassed. The tests themselves should not be randomly failing because of environmental problems. And lastly, the generated logfiles should designed with helping the reader find out where it went wrong.

https://issues.dlang.org/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&keywords=TestSuite%2C%20&keywords_type=allwords&list_id=232627&query_format=advanced

@WalterBright
Copy link
Member Author

By the way, I don't even know what "Azure pipelines (Windows x64)" even is. How is it different from the autotester doing the Win64 tests (which do work)? The documentation for the test doesn't say.

@WalterBright WalterBright force-pushed the fix21038 branch 4 times, most recently from 3abfb5f to f0ebe10 Compare August 8, 2020 06:11
@Geod24
Copy link
Member

Geod24 commented Aug 9, 2020

Fixing some bugs can be disruptive, and does one really want regressions in stable?

It can be, but do you think this is the case for this bug ?

But the current test suite has evolved [...]

All fair points, and no one disagree with you on this. We just need someone to do the job (and some people already stepped up).
To provide context, the evolution you mentioned were made so that we could improve things. On the topic of making the logs useful, the output diff feature added by @MoonlightSentinel would have been much harder to do if tests/run.d wasn't a D program. And IIRC the reason it switched from using the host compiler (which it originally was) to the built compiler was because DMD was running out of memory while building the code (which is both because the CI machine have limited memory and because DMD's memory consumption have only been going up for the last 5 years) and we needed to use the new -lowmem improvement.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I volunteeer to rebase it on stable if this is really required.
There's a badass conflict in mars1.d.

@ghost ghost added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Aug 13, 2020
@ghost
Copy link

ghost commented Aug 13, 2020

by seb comment

@dlang-bot dlang-bot merged commit c520e15 into dlang:master Aug 13, 2020
version (SCPP)
alignOffset(DATA, 2 << dt.DTalign);
version (MARS)
alignOffset(CDATA, 2 << dt.DTalign);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 1 << dt.DTalign, or is the alignment deliberately doubled?

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.

5 participants