xiaobai marked an inline comment as done. xiaobai added a comment. In D62755#1525790 <https://reviews.llvm.org/D62755#1525790>, @jingham wrote:
> This seems like carrying purity a little too far. I disagree that it is "carrying purity a little too far". My goal is to see LLDB's non-plugin libraries be more language agnostic. I have two motivations for this: 1. Better language support in LLDB, making it easier to add support for new languages in a clean fashion. 2. Shrinking the size of lldb-server. Though the lldb on llvm.org doesn't suffer from this problem too much, swift-lldb has an issue where the lldb-server you build is incredibly bloated. On Linux I am measuring about 162MB and on android I measured 61MB. It is incredibly difficult to shrink the size without first decoupling components. Because swift-lldb is a downstream project, I want to make sure that the abstractions there make sense with what is on llvm.org, so I am doing this work here first. > You haven't removed the fact that the using code is explicitly dialing up the > C++ language runtime, you've just made the call-site a little harder to read. Right, that was not an explicit goal. There are legitimate instances where you need to get the C++ language runtime, I'm not saying that this is a bad thing. I'm saying that if you need a CPPLanguageRuntime from your process, you should use `GetLanguageRuntime()` with LanguageType being eLanguageTypeC_plus_plus. If you need to call a method from CPPLanguageRuntime (or one its derived classes) that isn't a part of the LanguageRuntime interface, then you should explicitly cast it. I also think that this should happen in Plugins that need details about the C++ language runtime explicitly and not in non-plugin libraries. > I don't see any problem with having explicit accessors for the commonly used > language runtimes. Language runtimes are supposed to be plugins. I don't think Process needs to have knowledge about specific language runtime plugins. This particular instance is relatively harmless, but it is a necessary part of a larger change to decouple specific language details from the core lldb implementation. I recognize that CPPLanguageRuntime currently also lives in Target, but I would like to see it moved to "source/Plugins/LanguageRuntime/CPlusPlus/". I think that before I try to move it, this change makes sense as a first step. In D62755#1525810 <https://reviews.llvm.org/D62755#1525810>, @aprantl wrote: > I tend to agree with Jim's comment. > > > There is "GetLanguageRuntime()" which should be used instead. > > Can you explain why that is? I think my response to Jim above explains my reasoning here. If there's something I haven't addressed or you have other concerns, please let me know. ================ Comment at: source/Target/Process.cpp:1601 -CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) { - std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex); ---------------- aprantl wrote: > so what about this mutex at the new call sites? GetLanguageRuntime locks the mutex as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62755/new/ https://reviews.llvm.org/D62755 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
