shafik added a comment. In D67994#1697981 <https://reviews.llvm.org/D67994#1697981>, @labath wrote:
> In D67994#1695172 <https://reviews.llvm.org/D67994#1695172>, @shafik wrote: > > > In D67994#1692645 <https://reviews.llvm.org/D67994#1692645>, @labath wrote: > > > > > Maybe this is my fault since I'm the one who introduced the first bunch > > > of arguments here IIRC, but anyway, I have a feeling all of these dumping > > > options are getting out of hand. Looking at the argument list, you'd > > > expect that -dump-ast, and -dump-clang-ast do something completely > > > different, but they actually print the exact same AST, only one allows > > > you to print a subset of it, while the other one doesn't. > > > > > > Given that the ast dumping is currently incompatible with all/most of the > > > other dumping options anyway, maybe we should turn over a clean slate, > > > and implement a new subcommand specifically dedicated to dumping clang > > > ast (as opposed to the internal lldb representations of > > > types/functions/..., which is what the "symbols" subcommand started out > > > as, and which is what most of its options are dedicated to). > > > > > > OR, and this would-be super cool, if it was actually possible, we could > > > try to make ast-dumping compatible with the existing searching options. > > > Currently, if you type `lldb-test symbols -find=type -name=foo` it will > > > search for types named "foo", and print some summary of the lldb object > > > representing that type. What if we made it so that adding `-dump-ast` to > > > that command caused the tool to dump the *AST* of the types it has found > > > (e.g., in addition to the previous summary)? I think that would make the > > > behavior of the tool most natural to a new user, but I am not sure > > > whether that would actually fit in with your goals here... > > > > > > The output is actually quite different for example given: > > > > struct A { > > struct { > > int x; > > }; > > } a; > > > > > > the output of `lldb-test symbols -dump-ast anon.o` is: > > > > Module: anon.o > > struct A; > > > > > > while the output of `lldb-test symbols -dump-clang-ast anon.o` is: > > > > Module: anon.o > > A > > CXXRecordDecl 0x7ff3698262c8 <<invalid sloc>> <invalid sloc> struct A > > definition > > |-DefinitionData pass_in_registers aggregate standard_layout > > trivially_copyable pod trivial literal > > ... > > > > > > Given they are both working with the Symbol File I believe they both belong > > under the `symbol` command. I feel like `-dump-ast` is a little misleading > > but `-dump-clang-ast` feels good. > > > You're right about. I'm sorry, I should have checked what -dump-ast does. It > definitely is confusing that it does something completely different than the > "dump ast" lldb command. > > But what about the second part of my comment? I think it would still be nice > if the `lldb-test symbols` options could be somehow factored into two groups: > > - a group which selects *what* to dump: this would be `-find`, `-name`, etc. > - a group which selects *how* to dump it: -dump-clang-ast, -dump-ast :/, the > default mode > > would it be somehow possible to hook in this logic into the `findTypes` > function in lldb-test, so that instead of calling TypeMap::Dump, it does > something else if `-dump-clang-ast` flag is present (this "else" resuling in > the printing of clang ast)? I sympathize with the sentiment that it would be nice to collapse some of the arguments. I spent some time trying to see how to combine these features nicely and I don't think it will end up being any cleaner. Folding in the `-dump-ast` and `-dump-clang-ast` into `-find` only seems to make sense with `-find=type` or `-find=none` so this feels like shoe horning by lumping it with the `-find` option. Since I now need to add error handling for the blocks, variables and namespaces If you look at the switch for `Find` it already has a mess of conflicting options :-( for example `-find=block` is the only one that use `-line`. We won’t be saving much in the way of code just shifting where the if/else will be. Each functionality is really providing different information and I can see how each one is useful, so I don’t want to remove any of them. I do think the `-dump-ast` could use a better name maybe `-print-decl` because it end up using `DeclPrinter` while `-dump-clang-ast` end up using `ASTDumper`. This utility can be probably be refactored to be cleaner but that is a bigger change and outside the scope of being able to have the ability to verify fixes for how we generate clang ASTs from DWARF which we have no way of doing and I need now to be able to verify a fix I have now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67994/new/ https://reviews.llvm.org/D67994 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
