[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: source/Core/ModuleList.cpp:94
 
-  llvm::SmallString<128> path;
-  clang::driver::Driver::getDefaultModuleCachePath(path);
-  SetClangModulesCachePath(path);
+  assert(!g_default_clang_modules_cache_path.empty());
+  SetClangModulesCachePath(g_default_clang_modules_cache_path);
----------------
zturner wrote:
> aprantl wrote:
> > zturner wrote:
> > > I don't think this should be an assert.  After all, if the whole point is to make LLDB usable in situations where clang is not present, then someone using it in such an environment would probably never call the static function to begin with.  So I think we should just remove the assert and set it to whatever the value happens to be (including empty)
> > The assertion enforces that ModuleListProperties::Initialize() has been called. If we want to make it more convenient, we can add a default argument `= "dummy"` for clients that don't link against clang.
> I was actually thinking that instead of calling it `Initialize` (which sounds generic and like it's required) we would just call it `SetDefaultClangModulesCachePath` and have the user directly call that.  With a name like `Initialize`, it makes the user think that it's required, but in fact the only thing it's initializing is something that is optional, so it shouldn't be required.
> 
> It's possible I'm misunderstanding something though.
My point was that this *is* required (for all clients of lldb that also link against clang). When LLDB initializes clang it must set a module cache path because clang doesn't implement a fallback.


https://reviews.llvm.org/D47235