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

zturner added a comment. That sounds good to me as well. https://reviews.llvm.org/D47235 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

labath added a comment. Moving the assertion into `ClangModulesDeclVendor` seems like a good idea to me. If that's the only place that actually requires the value to be non-empty, then it should be the thing asserting it (we can put a helpful message in the assert statement telling the user

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

> On May 23, 2018, at 1:33 PM, Zachary Turner wrote: > > Actually, now I’m starting to wonder why can’t ClangModulesDeclVendor.cpp > just call this function in clangDriver to get the default if the accessor > returns an empty string? That sidesteps all of this

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

zturner added a comment. Actually, now I’m starting to wonder why can’t ClangModulesDeclVendor.cpp just call this function in clangDriver to get the default if the accessor returns an empty string? That sidesteps all of this initialization funny business and lets the client just handle it.

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

Actually, now I’m starting to wonder why can’t ClangModulesDeclVendor.cpp just call this function in clangDriver to get the default if the accessor returns an empty string? That sidesteps all of this initialization funny business and lets the client just handle it. Would that work? On Wed,

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

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()); +

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

aprantl updated this revision to Diff 148265. aprantl added a comment. Remove unused include. https://reviews.llvm.org/D47235 Files: include/lldb/API/SystemInitializerFull.h include/lldb/Core/ModuleList.h source/API/SystemInitializerFull.cpp source/Core/ModuleList.cpp

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

aprantl updated this revision to Diff 148264. aprantl added a comment. Remove whitespace changes. https://reviews.llvm.org/D47235 Files: include/lldb/API/SystemInitializerFull.h include/lldb/Core/ModuleList.h source/API/SystemInitializerFull.cpp source/Core/ModuleList.cpp

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

zturner added a comment. Can you revert the changes to `HostInfoBase.h`? It looks like now it's only whitespace changes. Comment at: source/Core/ModuleList.cpp:16 #include "lldb/Host/Symbols.h" +#include "lldb/Host/HostInfoBase.h" #include

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

aprantl updated this revision to Diff 148248. aprantl added a comment. Forgot to update unit tests. https://reviews.llvm.org/D47235 Files: include/lldb/API/SystemInitializerFull.h include/lldb/Core/ModuleList.h include/lldb/Host/HostInfoBase.h source/API/SystemInitializerFull.cpp

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

aprantl updated this revision to Diff 148241. aprantl added a comment. Updated patch according to Zachary's suggestion. https://reviews.llvm.org/D47235 Files: include/lldb/Core/ModuleList.h include/lldb/Host/HostInfoBase.h source/API/SystemInitializerFull.cpp source/Core/ModuleList.cpp

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

SystemInitializerFull.cpp, in the function SystemInitializerFull::Initialize(). On Wed, May 23, 2018 at 9:44 AM Adrian Prantl wrote: > > > On May 23, 2018, at 8:51 AM, Zachary Turner wrote: > > There's not really a diagram, because we don't have an exact

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

> On May 23, 2018, at 8:51 AM, Zachary Turner wrote: > > There's not really a diagram, because we don't have an exact vision of what > the final layering is going to look like (some things will need to be split > up, entirely new targets will need to be introduced, etc).

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

labath added a comment. In https://reviews.llvm.org/D47235#1109580, @zturner wrote: > In https://reviews.llvm.org/D47235#1109219, @labath wrote: > > > I guess it would be nice to encapsulate this in some sort of a plugin > > (since the setting is used from the clang expression parser plugin, I

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

There's not really a diagram, because we don't have an exact vision of what the final layering is going to look like (some things will need to be split up, entirely new targets will need to be introduced, etc). Mostly it's just built from experience based on what the primary logical function of a

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

> On May 22, 2018, at 6:57 PM, Zachary Turner wrote: > > Yea I don’t think this addresses the problem. We should be able to link > against parts of lldb without a dependency on clang. Since this is about > configuring something related to clang, it seems like it should be

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

zturner added a comment. In https://reviews.llvm.org/D47235#1109219, @labath wrote: > In a way, this makes sense, but in a lot of other ways, it actually makes > things worse :) > > My long-term goal is to be able to build lldb-server without even having > clang checked out (because it doesn't

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

labath added a comment. In a way, this makes sense, but in a lot of other ways, it actually makes things worse :) My long-term goal is to be able to build lldb-server without even having clang checked out (because it doesn't really need anything clang-related), and this would certainly make

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

zturner added a subscriber: aprantl. zturner added a comment. Yea I don’t think this addresses the problem. We should be able to link against parts of lldb without a dependency on clang. Since this is about configuring something related to clang, it seems like it should be isolated to some part

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

Yea I don’t think this addresses the problem. We should be able to link against parts of lldb without a dependency on clang. Since this is about configuring something related to clang, it seems like it should be isolated to some part of lldb that interfaces with clang On Tue, May 22, 2018 at 4:32

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

aprantl created this revision. aprantl added reviewers: zturner, jingham. Herald added subscribers: jkorous, MaskRay, ioeric, ilya-biryukov, mgorny. @zturner wrote: > This change has introduced a dependency from Core -> clang Driver (due to > #include "clang/Driver/Driver.h" in ModuleList.cpp).