-
-
Notifications
You must be signed in to change notification settings - Fork 619
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
Add external import path switch #20899
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @rikkimax! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 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#20899" |
The switch itself needs a test, (although I have yet to figure out how to do it). What is here, is already covered by the testsuite. |
Why does this need a spec PR? This is for CLI switch, and that gets updated automatically based upon the table in the compiler. |
Documentation |
Which is automatically generated from the CLI switch table in the compiler. So what are you expecting in the spec to change? |
9b39c1e
to
350582a
Compare
Upon reflection, the existing DLL tests cannot be made to work. They require both druntime and phobos to exist as DLL's which are not current produced. I either have to implement a -betterC test for this from scratch, or put it in without a test. |
Upon review, the dllimport override switch would be required, as none would quit the process of determining DllImport status too early. I have also determined that cxx_dll test should work for the test case. As long as its just importing a single global variable. |
2ac3bcd
to
de96a44
Compare
I'm expecting on Windows the dll_cxx test to fail, I didn't add the |
There we go! We have confirmation that without |
I don't understand why Azure pipelines (Windows_DMD_bootstrap x64) is failing. It's not dll_cxx test. I'll need to review what I changed in the checks to make sure I didn't mess that up. |
Azure randomly fails sometimes |
Indeed, after review, I think that is the case here. It was a DLL test, which is why I got suspicious. |
Okay it's now green. The tags can be removed, there is a test @ibuclaw for GDC I am assuming you'll need to name it something like @kinke I'd appreciate a look, from what I see for ldc the dllimport detection logic is in two functions so its going to need a bit of work. If we need to discuss who does what for ldc we can later, there is no point doing it here until after a downstream. @rainers you wrote the code, so if you want to check it over, here is a ping. |
Before I attempt to add the switch itself, let's make sure I didn't break anything with my refactoring of the DllImport detection logic.
Based upon this logic, I can see that the
-dllimport=externalOnly
option wouldn't be required.I cannot foresee a reason why you would want to set the explicitly external flag on a module, without also wanting it to be DllImport.