[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

xiaobai added a comment. ping @jingham @teemperor any issues with this going in? I'd like to land this soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64844/new/ https://reviews.llvm.org/D64844 __

[Lldb-commits] [PATCH] D69820: [Symbol] Add TypeSystem::GetClassName

xiaobai updated this revision to Diff 232960. xiaobai added a comment. Address feedback from @teemperor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69820/new/ https://reviews.llvm.org/D69820 Files: lldb/include/lldb/Symbol/ClangASTContext.h

[Lldb-commits] [PATCH] D69820: [Symbol] Add TypeSystem::GetClassName

xiaobai added inline comments. Comment at: lldb/source/Core/ValueObject.cpp:2056 +if (parent_had_base_class) + s.PutCString("::"); +s.PutCString(class_name.getValue()); jingham wrote: > That's still implicitly C. Other languages (swift) use "." for

[Lldb-commits] [PATCH] D71232: [lldb/Lua] Add Boilerplate for a Lua Script Interpreter

xiaobai added a comment. I'm really excited to see where this goes! :D Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:64 +const char *ScriptInterpreterLua::GetPluginDescriptionStatic() { + return "Null script interpreter"; +} W

[Lldb-commits] [PATCH] D71234: [lldb/Lua] Implement a Simple Lua Script Interpreter Prototype

xiaobai added inline comments. Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/CMakeLists.txt:10 +find_package(Lua REQUIRED) +include_directories(${LUA_INCLUDE_DIR}) + Nit: I would use `target_include_directories` here. https://cmake.org/cmake/help/latest/

[Lldb-commits] [PATCH] D71237: [FormatEntity] Add mangled function name support

xiaobai added inline comments. Comment at: lldb/test/Shell/Settings/Inputs/main.cpp:95 + +//virtual +~C() nit: comment unneeded. Comment at: lldb/test/Shell/Settings/TestFrameFormatMangling.test:9 +frame info +# CHECK: frame #0: {{.*}}_

[Lldb-commits] [PATCH] D69820: [Symbol] Add TypeSystem::GetClassName

xiaobai updated this revision to Diff 233163. xiaobai added a comment. Add fixme Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69820/new/ https://reviews.llvm.org/D69820 Files: lldb/include/lldb/Symbol/ClangASTContext.h lldb/include/lldb/Symbo

[Lldb-commits] [PATCH] D71306: [RFC] Change how we deal with optional dependencies

xiaobai added a comment. I personally prefer the third approach. To make sure I understand correctly, I'll write it in my own words so you can correct me if I misunderstood. Try to find the dependency, and if we find it then use it. If not, then we can print out something like "Didn't find `DEPE

[Lldb-commits] [PATCH] D71306: [RFC] Change how we deal with optional dependencies

xiaobai added a comment. In D71306#1779379 , @labath wrote: > In D71306#1778472 , @xiaobai wrote: > > > I personally prefer the third approach. To make sure I understand > > correctly, I'll write it in my own words

[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

xiaobai added a comment. Landed as commit 3031818a2e9fca1e53cd882ccfcc371861b4 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64844/new/ https://reviews.llvm.org/D64844 __

[Lldb-commits] [PATCH] D72107: [lldb/CMake] Autodetect Python dependency

xiaobai added a comment. It's nice to consolidate the logic into one place. I think you will probably need to make an appropriate change on the buildbot side as well (if you haven't done that already). Where is `FindPythonInterpAndLibs` being included? Repository: rLLDB LLDB CHANGES SINCE

[Lldb-commits] [PATCH] D72107: [lldb/CMake] Autodetect Python dependency

xiaobai added a comment. In D72107#1802029 , @JDevlieghere wrote: > Do you mean forcing `LLDB_ENABLE_PYTHON` to on so that it fails in case the > logic changes and Python isn't found? Yes, something like that. It looks like the previous expected behavi

[Lldb-commits] [PATCH] D72541: [lldb/Utils] Remove vim-lldb

xiaobai accepted this revision. xiaobai added a comment. This revision is now accepted and ready to land. The only things that have touched vim-lldb in the past 5-6 years have been the result of repository-wide changes (e.g. reformatting, documentation changes, python compatability and version t

[Lldb-commits] [PATCH] D69820: [Symbol] Add TypeSystem::GetClassName

xiaobai updated this revision to Diff 237445. xiaobai added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69820/new/ https://reviews.llvm.org/D69820 Files: lldb/include/lldb/Symbol/ClangASTContext.h lldb/include/lldb/Symbol/T

[Lldb-commits] [PATCH] D69820: [Symbol] Add TypeSystem::GetClassName

xiaobai added a comment. In D69820#1817621 , @clayborg wrote: > Let me know what everyone thinks of adding a "fully_qualified" argument to > the TypeSystem::GetClassName()? I think that you would have to add more than a `fully_qualified` argument to th

[Lldb-commits] [PATCH] D72684: [lldb][NFC] Rename ClangASTContext to TypeSystemClang

xiaobai added a comment. I like this idea quite a bit, but have no preference for `ClangTypeSystem` or `TypeSystemClang`. +1 from me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72684/new/ https://reviews.llvm.org/D72684 ___ lldb-commits

[Lldb-commits] [PATCH] D72684: [lldb][NFC] Rename ClangASTContext to TypeSystemClang

xiaobai added a comment. In D72684#1820486 , @clayborg wrote: > We might also want to move these into lldb/source/Plugins/TypeSystem as well > to complete this refactor? You could move it now, but ValueObject still depends on it (and you would be intro

[Lldb-commits] [PATCH] D72946: [lldb] Remove ClangASTImporter reference from Target

xiaobai created this revision. xiaobai added reviewers: JDevlieghere, teemperor, labath, clayborg. Herald added a reviewer: martong. Herald added a reviewer: a.sidorin. Herald added a reviewer: shafik. Herald added a project: LLDB. Target is one of the classes responsible for vending ClangASTImpor

[Lldb-commits] [PATCH] D72946: [lldb] Remove ClangASTImporter reference from Target

xiaobai added a comment. In D72946#1827301 , @teemperor wrote: > I wish we could do this without a global map. Also the ClangASTImporter > shouldn't have a dependency on Target (I'm actually surprised this compiles > without an additional include). > > I

[Lldb-commits] [PATCH] D72946: [lldb] Remove ClangASTImporter reference from Target

xiaobai updated this revision to Diff 240001. xiaobai added a comment. Implement @teemperor's suggestion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72946/new/ https://reviews.llvm.org/D72946 Files: lldb/include/lldb/Symbol/TypeSystemClang.h

[Lldb-commits] [PATCH] D73517: [lldb] Delete ValueObject::GetBaseClassPath

xiaobai created this revision. xiaobai added reviewers: teemperor, labath, clayborg, JDevlieghere. Herald added a project: LLDB. This method has exactly one call site, which is only actually executed if `ValueObject::IsBaseClass` returns false. However, the first thing that `ValueObject::GetBaseCl

[Lldb-commits] [PATCH] D73517: [lldb] Delete ValueObject::GetBaseClassPath

xiaobai added a comment. Note: This makes D69820 unnecessary so I will close that if this goes through. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73517/new/ https://reviews.llvm.org/D73517 _

[Lldb-commits] [PATCH] D72946: [lldb] Remove ClangASTImporter reference from Target

xiaobai updated this revision to Diff 240762. xiaobai added a comment. Sort headers via clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72946/new/ https://reviews.llvm.org/D72946 Files: lldb/include/lldb/Symbol/TypeSystemClang.h lld

[Lldb-commits] [PATCH] D72946: [lldb] Remove ClangASTImporter reference from Target

xiaobai closed this revision. xiaobai added a comment. Commit c4f6fbe971351273b19a4a819bf6ceae2b70b37e Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72946/new/ https://reviews.l

[Lldb-commits] [PATCH] D69820: [Symbol] Add TypeSystem::GetClassName

xiaobai closed this revision. xiaobai added a comment. Made obsolete by D73517 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69820/new/ https://reviews.llvm.org/D69820 ___

[Lldb-commits] [PATCH] D73517: [lldb] Delete ValueObject::GetBaseClassPath

This revision was automatically updated to reflect the committed changes. Closed by commit rG5eaf44f99f0a: [lldb] Delete ValueObject::GetBaseClassPath (authored by xiaobai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73517/new/ https://reviews.ll

[Lldb-commits] [PATCH] D73661: [lldb] Move clang-based files out of Symbol

xiaobai added a comment. In D73661#1848750 , @labath wrote: > I think this is a big milestone. Thanks for working on this. > > The main question I have is about the new location of this code. This patch > puts it under ExpressionParser/Clang, which is no

[Lldb-commits] [PATCH] D73517: [lldb] Delete ValueObject::GetBaseClassPath

xiaobai added inline comments. Comment at: lldb/source/Core/ValueObject.cpp:2041 void ValueObject::GetExpressionPath(Stream &s, bool qualify_cxx_base_classes, GetExpressionPathFormat epformat) { labath wrote: > Should we re

[Lldb-commits] [PATCH] D73661: [lldb] Move clang-based files out of Symbol

This revision was automatically updated to reflect the committed changes. Closed by commit rG8be30215feef: [lldb] Move clang-based files out of Symbol (authored by xiaobai). Changed prior to commit: https://reviews.llvm.org/D73661?vs=241584&id=241791#toc Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D73827: [lldb] Delete ClangForward.h

xiaobai created this revision. xiaobai added a reviewer: LLDB. Herald added subscribers: usaxena95, arphaman. Herald added a project: LLDB. I think that there are very few things from clang that actually need forward declaration, so not having a ClangForward header makes sense. Repository: rG

[Lldb-commits] [PATCH] D73827: [lldb] Delete ClangForward.h

This revision was automatically updated to reflect the committed changes. Closed by commit rG5b0c8dd3a4fd: [lldb] Delete ClangForward.h (authored by xiaobai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73827/new/ https://reviews.llvm.org/D73827

[Lldb-commits] [PATCH] D73935: [lldb] Remove clang classes from lldb-forward.h

xiaobai created this revision. xiaobai added a reviewer: LLDB. Herald added a project: LLDB. lldb-forward.h is convenient in many ways, but having clang-based class forward declarations in there makes it easy to proliferate uses of clang outside of plugins. Removing them makes you much more consci

[Lldb-commits] [PATCH] D73935: [lldb] Remove clang classes from lldb-forward.h

This revision was automatically updated to reflect the committed changes. Closed by commit rG7c9ebdd3d6ae: [lldb] Remove clang classes from lldb-forward.h (authored by xiaobai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73935/new/ https://review

[Lldb-commits] [PATCH] D74187: [lldb] Add method Language::IsMangledName

xiaobai created this revision. xiaobai added a reviewer: LLDB. Herald added a project: LLDB. Instead of asking each of the language plugins directly if a symbol is a possible manghled name for that language, we can generalize this pattern. By adding a method `IsMangledName` and iterating over each

[Lldb-commits] [PATCH] D74187: [lldb] Add method Language::IsMangledName

xiaobai marked an inline comment as done. xiaobai added a comment. I uploaded this patch primarily to get some feedback on possible alternatives because I'm not happy creating a dependency from `Core` to `Target` here. Suggestions welcome! Comment at: lldb/source/Core/Mangled

[Lldb-commits] [PATCH] D74187: [lldb] Add method Language::IsMangledName

xiaobai added inline comments. Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:96-98 + bool IsMangledName(llvm::StringRef name) const override { +return false; + } friss wrote: > The original code was calling `IsPossibleObjCMethodName` and it l

<    9   10   11   12   13   14