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
Revert "Move a couple of functions out of dsymbol.d (#15789)" #15854
Conversation
This reverts commit 7bff453.
Thanks for your pull request, @ibuclaw! Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#15854" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've put a GDC tag on it this time, which is better than nothing, but I'd still like an explanation as to why this is necessary, considering that this undies progress of moving stuff out of AST and into sema. Again I suspect that the appropriate course of action here is to make this work not to revert it.
Please do not merge this without a satisfactory explanation.
|
|
no justification for keeping regression introducing refactorings.
So the linker errors are the same as in the case of the other PR? This looks like something that is particular to the way gdc compiles the frontend. |
Modules are compiled separately, there's nothing peculiar about it. |
Clearly it is 1) different and, 2) not tested. You should set up a CI job that builds and links DMD with modules copied separately if you want to prevent this from happening again. |
You can watch the daily builds. https://github.com/D-Programming-GDC/gcc/actions/runs/6982207333 Downstream user noticed that it affects everything, not just the target they were bootstrapping from. I've accounted for this and now you see everything is red. |
I meant a DMD CI job, so that it gets caught here and not later on. |
DMD CI is not a good yardstick for success. |
Blocked for a week, and there's yet another pr that pushes the diff with upstream over the 400kb limit. |
What limit? |
The change is quite small in the mentioned PR, it's just that the automated generation of Anyway, I can put up a PR to unblock you in a way that does not undo previous work, however, since building gdc is not tested on the pipeline, I have no way of knowing it my fix actually does something. |
By the way, this broke before in the same way due to functions being moved around. https://issues.dlang.org/show_bug.cgi?id=21294 (Fix PR #11827) To quote an old comment of mine I forgot I made:
In this change, it's instead failing for |
There's already a test in the testsuite for this - the problem is DMD has been patched to address the issue since 2.094.1. |
@ibuclaw is it ok to close this? |
Mainline is green again. https://github.com/D-Programming-GDC/gcc/actions/workflows/main.yml |
This reverts commit 7bff453.