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

FileManager redux: replace FileCache with FileManager #13193

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

ellie-idb
Copy link
Contributor

@ellie-idb ellie-idb commented Oct 18, 2021

This PR seeks to take the work in #12037 and revive the work done to replace the near-useless FileCache struct in dmd/filecache.d.

Why?

filecache.d has a lot of issues. It:

  • uses some rather dangerous pointer arithmetic to split a file by lines (ew, we have slices for a reason, there should be no reason for pointer arithmetic)
  • only supports reading files from disk
  • revealed several issues in my previous PR where I seeked to enable -verrors=context by default (Enable -verrors=context by default #13174), related to the fact that it only supported reading files from disk (i.e. dmd.errors.verrorPrint throwing an exception because it was not able to find __stdin.d)
  • seems to be completely unmaintained and has remained virtually untouched
  • is memory inefficient (results in two copies of one file being stored in memory)
  • didn't have a corresponding C++ header, even though C++ users of the front-end needed to initialize it for -verrors=context to work

The work done in #12037 was very much on the right track, but was abandoned (seemingly due to a lack of interest or something else). This seeks to address all of the issues with filecache.d, and integrate it into the compiler (taking into account all criticism that was given to the PR).

With merging this PR:

  • more work can be done on more developer-friendly error printing
  • should also somewhat reduce disk I/O on bigger compiles, as one access is generally all that would be needed (unless some developer decides to steal your buffer with extractSlice) -- what benefit this has is TBD
  • the content of files will generally be available for users of the front-end to access (for an LSP, or whatnot)

Do let me know if this needs further unittesting.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @hatf0! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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

Your 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 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#13193"

@ellie-idb ellie-idb changed the title FileManager redux FileManager redux: replace FileCache with FileManager Oct 18, 2021
@ibuclaw
Copy link
Member

ibuclaw commented Oct 18, 2021

didn't have a corresponding C++ header, even though C++ users of the front-end needed to initialize it for -verrors=context to work

Actually, gdc is a cool kid and integrates with gcc diagnostics, not the loser dmd ones, so keep it private to D please.

src/dmd/frontend.h Outdated Show resolved Hide resolved
src/dmd/mars.d Outdated Show resolved Hide resolved
src/dmd/mars.d Show resolved Hide resolved
src/dmd/root/file_manager.d Outdated Show resolved Hide resolved
dub.sdl Outdated Show resolved Hide resolved
src/dmd/frontend.h Outdated Show resolved Hide resolved
src/dmd/root/file_manager.d Outdated Show resolved Hide resolved
src/dmd/root/file_manager.d Outdated Show resolved Hide resolved
src/dmd/root/file_manager.d Outdated Show resolved Hide resolved
src/dmd/root/file_manager.d Outdated Show resolved Hide resolved
src/dmd/root/file_manager.d Outdated Show resolved Hide resolved
src/dmd/mars.d Show resolved Hide resolved
src/dmd/mars.d Outdated Show resolved Hide resolved
src/dmd/frontend.h Outdated Show resolved Hide resolved
@ellie-idb ellie-idb force-pushed the filemanager-redux branch 2 times, most recently from b4c083b to 862b4f5 Compare October 18, 2021 16:21
@ellie-idb
Copy link
Contributor Author

@ibuclaw added the requested fixes -- let me know what you think

@ellie-idb
Copy link
Contributor Author

FWIW, for anyone wondering, it looks like farm-4 on the auto-tester is just flat-out broken.

@RazvanN7
Copy link
Contributor

cc @ibuclaw @kinke

@ellie-idb
Copy link
Contributor Author

Rebased on master -- @ibuclaw @kinke any objections?

@kinke
Copy link
Contributor

kinke commented Oct 26, 2021

Sorry, no time for properly reviewing this, but after an extremely superficial glance, it doesn't look like LDC will be affected (it has a somewhat modified mars.d and will include the few lines).

@RazvanN7 RazvanN7 merged commit 469890b into dlang:master Oct 27, 2021
@ibuclaw
Copy link
Member

ibuclaw commented Feb 22, 2022

This PR caused a regression https://issues.dlang.org/show_bug.cgi?id=22816

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.

5 participants