jingham requested changes to this revision. jingham added inline comments. This revision now requires changes to proceed.
================ Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:119 } + +llvm::ArrayRef<OptionDefinition> ---------------- There's a version of Append that lets you exclude and remap option sets. This seems like a finer-grained version of the same thing where you just want to exclude particular options by name but not mess with the option sets. That seems a generally useful thing to do, so it would be better to make another flavor of Append that takes an array of short character options and just excludes them, than do it as a one-off here. ================ Comment at: lldb/source/Commands/CommandObjectExpression.h:38 + /// Return the appropriate expression options used for evaluating the + /// expression in the given target. ---------------- Maybe this should instead take an OptionGroupValueObjectDisplay& instead? I think it's clearer to say "some of the EvaluateExpressionOptions are governed by how you want to display the result ValueObject you're going to produce" than "I'm passing a couple separate ValueObjectDisplay options under the table." Plus, that way if you end up needing another display option you won't have to keep adding parameters to this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144114/new/ https://reviews.llvm.org/D144114 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
