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 17699 - Importing a module that has both modulename.d and modulename/package.d should be an error #14407
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @dkorpel! 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 references
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#14407" |
a8c3ec0
to
bb6d539
Compare
@@ -0,0 +1,12 @@ | |||
/* |
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.
Add EXTRA_FILES for documentation purposes (and the gdc dejagnu testsuite)
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.
Done
Should it, really ? I don't see it as particularly problematic, and the solution is to add one FS access for every module we open, which is IMO pretty bad. |
What happens if you |
See the bugzilla issue for rationale.
How much do you think it affects build times? |
The same as before.
I don't think they affect anything here. |
Yeah that rationale is shaky. "If one version of Phobos is unzipped over another" isn't a great start, as it suffers from numerous other bugs as soon as the structure is touched.
This instance, probably little, however it's the little things that matter. Any time we have to do an extra check / some extra work in the general case for a corner case, we should ask ourselves if it's the right thing to do. This case being, IMO, a textbook example. |
I recall encountering a segfault when reducing something unrelated a few months back, didn't have time to synthesize. This file structure looked familiar though. |
Textbook example of what? Do you have suggestions for a different implementation or should the issue just be closed as WONTFIX? |
|
Rationale written in 2017, when a common mechanism to install your new compiler was to unzip it over the old one.
We are already opening multiple for a Let's say you have a dub project with no dependencies. importing std.stdio stats 12 files before it eventually opens the right one (tested with strace):
Add 5 more stats for each dependency. Adding one more stat doesn't seem like it will make a huge difference in the performance. |
2b4c6bc
to
dbcd8eb
Compare
…ame/package.d should be an error
dbcd8eb
to
be46270
Compare
In this case I agree with @Geod24 . Even if the penalty is small compared to what is already being done, each little corner case check adds to the churn. On the hand, the benefit is almost non existent in my opinion, since the frequency of such a scenario is very small. |
Please measure. If this is going to add a few ns per import, I think it's worth avoiding causing a 1 hour puzzlement when it does happen. Note, ImportC was worth adding 2N extra stats (two per import path), and nobody blinked. This is one extra stat total, regardless of where the import is found. |
My point was more about adding ugliness to support a narrow use case, rather than performance. I don't think that the compiler should take care of every esoteric use case that arises. That being said, I really don't care that much about this case. If others decide that this is worth it, I'm not going to block this addition in any way. |
FYI, #14582 reduces the number of |
No description provided.