Rebase

In D79823#2032827, @davide wrote:

I'm curious what happens if you end up here with something like _T. Did you check?

In D76945#1947034, @JDevlieghere wrote:

In D76945#1946988, @xiaobai wrote:

we need to build our own and tell LLVM to use it

To clarify, this means we'd be building our own llvm-lit and using that just for the LLDB test suite, correct?

Yep, should I reword the comment?

we need to build our own and tell LLVM to use it

Thanks for doing this. LGTM, might want to wait for Jason's approval like Jonas said though.

LGTM. If UnwindLLDB is the unwinder, that makes things a little easier for plugin separation.

Thanks for working to make LLDB more language-agnostic!

I like the idea! The one thing that kind of bothers me about this is you can inadvertently break your build without realizing by disabling a plugin that some enabled plugins depend on. Hopefully one day we'll be able to improve the diagnostics around this.

I like where this is going!

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!

Move TypeSystemClang into its own plugin

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 not completely unreasonable, as that's where most of the clang stuff is. However, it does create some awkward-looking (to me) dependencies, or even loops (SymbolFileDWARF<->ExpressionParserClang). Also the dep from data formatters in Language/CPLusPlus on ExpressionParser/Clang is odd, because the data formatters don't actually use/need expressions do to their work.

So, as much as I hate proliferating plugins, I have to ask this question: Should this be a new plugin kind (TypeSystem/Clang) instead ? The name of the class, and the presence of the Initialize function already seem to indicate that. And it seems to me like this would create a reasonable dependency graph between the various plugins. TypeSystemClang would be at the bottom of this. The various SymbolFile plugins would depend on it, because they generate clang ASTs. ExpressionParserClang would depend on it because it pulls the generated ASTs that way. The same goes for the data formatters. But there would be no dependency between SymbolFiles and expression parsers or data formatters, because the former should just provide the ast (and not care about who the consumer is) and the latter should consume it (regardless of the source).

I think this ExpressionParser vs. TypeSystem would also make sense in terms of the outgoing dependencies. The type system should depend +/- only on clangAST, whereas the expression parser would need pretty much the whole clang.

WDYT?

Made obsolete by D73517

Commit c4f6fbe971351273b19a4a819bf6ceae2b70b37e

Sort headers via clang-format

Note: This makes D69820 unnecessary so I will close that if this goes through.

Implement @teemperor's suggestion

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'm not sure where the perfect place for the Target's ClangASTImporter is, but putting it in the ClangPersistentVariables would solve your problem of getting it out of Target and doesn't need a global map. Also in general the ClangASTImporter of the Target is used for copying stuff to the scratch context, so having it in the ClangPersistentVariables makes some sense.

In D72684#1820486, @clayborg wrote:

We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?

I like this idea quite a bit, but have no preference for ClangTypeSystem or TypeSystemClang. +1 from me.

In D69820#1817621, @clayborg wrote:

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

Rebase

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 transitions, etc).

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?

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).

Adding @vsk since he added the code that you're referencing in your summary.

Landed as commit 3031818a2e9fca1e53cd882ccfcc3718699991b4

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 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 DEPENDENCY" and continue on our merry way. If the user overwrites the values and something goes wrong, send a fatal error and tell them that what the value they set isn't going to work without further work (e.g. explicitly enable python support but didn't find python? tell the user that you couldn't find python and maybe suggest setting some other CMake variables to help CMake find python).

How exactly does this "overwriting" work? Could you point me to the code that does this? I don't remember seeing anything like this, but the llvm build is not entirely consistent either, so it's possible we're looking at different things...

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 DEPENDENCY" and continue on our merry way. If the user overwrites the values and something goes wrong, send a fatal error and tell them that what the value they set isn't going to work without further work (e.g. explicitly enable python support but didn't find python? tell the user that you couldn't find python and maybe suggest setting some other CMake variables to help CMake find python).

Add fixme

I'm really excited to see where this goes! :D

Address feedback from @teemperor

ping @jingham @teemperor any issues with this going in? I'd like to land this soon.

Rebase

Rebased
Moved this change to the monorepo layout
Moved ClangASTContext::GetScratch to the ClangASTContext header