zturner 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: > zturner wrote: > > aprantl wrote: > > > 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. > > If there's a client of LLDB using the public API and/or clang then that > > client would also be using `SystemInitializerFull` (or at the very least, > > would be responsible for initializing the set of things they need, one of > > which would be this path). > > > > My point is that `Core` should ultimately have no knowledge that something > > called clang even exists, and it definitely shouldn't be limiting the use > > of itself based on the needs of a specific client since it something that > > is useful to all clients. If a particular client requires clang, that > > client should initialize clang. > > > > With an assert, this is requiring a non clang-based client to run some > > initialization code that is only required for a clang-based client, which > > doesn't seem like a reasonable restriction (imagine if every downstream > > developer using every possible set of random 3rd party libraries started > > asserting in low-level debugger code that their optional component had been > > initialized). > In short, `Core` is too low level to be making any assumptions whatsoever > about the needs of a particular client. It may be required for all clients > of lldb that use clang, but `Core` is not the right place to be making > decisions based on whether a client of lldb uses clang (or any other optional > external library / component). To put this in perspective, imagine if LLVM's optimization pass library had something like `assert(driverIsClang());` https://reviews.llvm.org/D47235 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
