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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73661/new/

https://reviews.llvm.org/D73661