xiaobai added a comment. In D73661#1848750 <https://reviews.llvm.org/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? I think that creating a TypeSystem plugin is probably the better option here. After looking more closely at the dependency graph I think that what you pointed out is enough to justify a new plugin. I'll update this patch. In D73661#1848981 <https://reviews.llvm.org/D73661#1848981>, @teemperor wrote: > Also this doesn't compile on Linux: Thanks for checking this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73661/new/ https://reviews.llvm.org/D73661 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
