[PATCH] D80525: [clangd] Fix crash-bug in preamble indexing when using modules.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/TestFS.h:51
+  // This is useful for testing module support.
+  bool OverlayRealFileSystem = false;
 };
----------------
also ForModules here?


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.h:70
+  // Whether to use overlay the TestFS over the real filesystem. This is
+  // required for use of modules.
+  // FIXME: Intercept the write of implicitly build module to disk and update
----------------
nit: "of implicit modules, where the module file is written to disk and later read back"


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.h:71
+  // required for use of modules.
+  // FIXME: Intercept the write of implicitly build module to disk and update
+  // TestFS with that content to avoid using real file system in tests.
----------------
nit: I'm not sure the (V)FS should be involved at all in the final test setup.
The way I see it, the module storage is basically a cache from (module name + validity-stuff) ==> bytes, and *reads* could be abstracted just like writes.
For tests this could just be an in-memory map of course, but I think even in production we'd benefit a bit from not having to express this map through the filesystem. (E.g the way it's versioned doesn't have much to do with the way versioned filesystems are)

Only actionable thing for now is to maybe tweak this comment slightly, but wanted to mention it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80525/new/

https://reviews.llvm.org/D80525