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: > aprantl wrote: > > zturner wrote: > > > 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());` > > The assertion is not supposed to check that Clang has been initialized. It > > is supposed to check that ModuleListProperties::Initialize() has been > > called. The fact that in order to call this function a client may want to > > get a string from the Clang Driver is an (ugly) implementation detail. And > > clients that don't use clang (such as the confusingly named unit tests) can > > pass in any nonempty string (which as I offered earlier could be made into > > a default argument). > But why must it even be a nonempty string? And for that matter, if they're > not going to use clang anyway, why even require the function to be called in > the first place? If it were an initialization function that did multiple > things, it might be a stronger argument. But as it stands, its only purpose > is, in fact, to set a value for this path, which people who aren't using > clang shouldn't be required to do. > > This is making a decision in a low level library for the purpose of 1 > specific client, which doesn't seem right. I'm not entirely opposed to an > assert, but it should only happen in clients that are using clang, otherwise > this is effectively 'assert that the user has executed a no-op', which > doesn't make sense. > I'm not entirely opposed to an assert, but it should only happen in clients > that are using clang (and hence not in `Core` but in something higher level like ClangASTContext, or the place where you actually make use of this path). https://reviews.llvm.org/D47235 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
