sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1509
+TEST_F(SymbolCollectorTest, UndefOfModuleMacro) {
+ auto TU = TestTU::withCode(R"cpp(#include "/bar.h")cpp");
+ TU.AdditionalFiles["/bar.h"] = R"cpp(
----------------
are the leading slashes here needed, or can we use "bar.h" and have everything
be relative to testRoot()?
(relative paths in AdditionalFiles are relative to testRoot().)
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1523
+ TU.ExtraArgs.push_back("-fmodule-map-file=/module.map");
+ TU.OverlayRealFileSystem = true;
+
----------------
I'm a bit torn on this - this is obviously a bit messy (writing modules to the
real filesystem and exposing the whole FS to our test).
It's not a huge thing and we could slap a FIXME on it, though you clearly have
to plumb it through a few layers.
I think the alternative is an explicit module build:
- subclass `clang::GenerateHeaderModuleAction`to override the output so it
ends up in a buffer
- add a method to TestTU like buildHeaderModule() that returns a `std::string`
containing the module content
- This test would have two TestTU instances, and the second looks something
like:
```
TU2.AdditionalFiles["foo.pcm"] = TU1.buildHeaderModule();
TU2.ExtraArgs = {"-fprebuild-module-path=.", "-std=c++20", "-fmodules"};
TU2.HeaderCode = "import foo;\n#undef X";
```
I guess whether this is actually better probably depends on whether we're
likely to go in this direction (explicitly building and managing a module cache
ourselves) for modules. If we're likely to let clang implicitly build modules
and maybe just put the physical cache storage behind an abstraction, maybe it's
best to overlay the FS for now and rip this out later. If we're going to do
explicit module builds and schedule them ourselves, then starting to experiment
in that direction is useful, and there won't be an obvious time to rip out the
OverlayRealFileSystem feature.
I think all of which is to say feel free to land it in this form, or experiment
further.
I think the OverlayRealFileSystem should maybe be `ExposeModuleCacheDir` or
something with a FIXME to come up with a better solution, though. It's not a
feature of TesttU we should be using for other purposes (without similar
careful consideration)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80525/new/
https://reviews.llvm.org/D80525
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
