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

Simplify Module.read and its interaction with FileManager #13828

Merged
merged 4 commits into from
Mar 17, 2022

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Mar 17, 2022

Since the FileManager will read a file if it exists on the filesystem,
the else clause of Module.read was only ever called if File.read would fail.
As a result, the code can be simplified, and loadSourceBuffer can be renamed
to match its true purpose (printing a decent error message on missing file).

@Geod24 Geod24 requested a review from ibuclaw as a code owner March 17, 2022 03:27
@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 17, 2022

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close Bugzilla Severity Description
17167 blocker dmd fails to write to file or create directory with more than 248 characters in the path

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

@thewilsonator
Copy link
Contributor

compilable\issue17167.sh seems to fail everywhere on Azure

@Geod24
Copy link
Member Author

Geod24 commented Mar 17, 2022

Yep, legit failure, looking into it.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Check headers, modules.h, file.h, etc. For declarations that should be removed.

@Geod24
Copy link
Member Author

Geod24 commented Mar 17, 2022

@ibuclaw : FileBuffer is still a member of class Module so it can't be removed yet.
frontend.h removes it because its decl is no longer used. A future PR will remove it as a member of Module (I'm basically trying to get rid of the coupling between module and IO).

@Geod24 Geod24 force-pushed the fileman-part-1 branch 2 times, most recently from e1a520a to 853c8d8 Compare March 17, 2022 07:40
Geod24 added 2 commits March 17, 2022 16:43
While attempting to refactor Module.read not to call 'File.read' twice,
test17167 started to fail. The only possible difference was that
the first File.read call was conditional on FileName.exists succeeding,
while the second call was inconditional.

After a bit of research, it turns out that the fix for issue 17167
was apparently missing one key detail (the prefix) to enable long path.
This was not visible at the time because the fix for File.read was complete,
and so the second attempt would succeed, hiding the issue.

This brings into question the correctness / need for toWStringzThen,
as it's mostly used for long path, and is still being used in cannonicalName.
Since the FileManager will read a file if it exists on the filesystem,
the else clause of Module.read was only ever called if File.read would fail.
As a result, the code can be simplified, and loadSourceBuffer can be renamed
to match its true purpose (printing a decent error message on missing file).
@Geod24
Copy link
Member Author

Geod24 commented Mar 17, 2022

Found (and fixed) the bug

@Geod24
Copy link
Member Author

Geod24 commented Mar 17, 2022

To answer @ibuclaw more precisely:

% grep -nR -e "struct File$" -e "struct FileBuffer" $(find src -name "*.h")
src/dmd/frontend.h:232:struct FileBuffer;
src/dmd/root/file.h:15:struct FileBuffer
src/dmd/root/file.h:25:struct File
src/dmd/module.h:17:struct FileBuffer;

I could remove root/file.h if you guys are not using it, but perhaps it's better to wait until it's not used as a member of an AST node ? :)

@ibuclaw
Copy link
Member

ibuclaw commented Mar 17, 2022

To answer @ibuclaw more precisely:

% grep -nR -e "struct File$" -e "struct FileBuffer" $(find src -name "*.h")
src/dmd/frontend.h:232:struct FileBuffer;
src/dmd/root/file.h:15:struct FileBuffer
src/dmd/root/file.h:25:struct File
src/dmd/module.h:17:struct FileBuffer;

I could remove root/file.h if you guys are not using it, but perhaps it's better to wait until it's not used as a member of an AST node ? :)

It could be a void* field. Having a quick scan, the C++ driver still needs to set srcBuffer->data when creating a module for code read from stdin. If there was a C++ helper method such as Module::setSrcBuffer(char *ptr, size_t length) then you could internalize File and FileBuffer, removing file.h outright.

Just making sure that the C++ interface has been checked against for the removals here.

@ibuclaw ibuclaw dismissed their stale review March 17, 2022 09:38

Headers have been checked.

@Geod24 Geod24 merged commit 62e74ba into dlang:master Mar 17, 2022
@Geod24 Geod24 deleted the fileman-part-1 branch March 17, 2022 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants