[Lldb-commits] [PATCH] D46318: lldb-test symbols: Add ability to do name-based lookup

Jonas Devlieghere via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed May 2 03:28:50 PDT 2018

JDevlieghere added a comment.

A few nits/stylistic comments

Comment at: tools/lldb-test/lldb-test.cpp:86
+static cl::opt<LookupType> Lookup(
+    "lookup", cl::desc("Choose lookup type:"),
It's a bit unfortunate that these argument names are different from the ones we have in dwarfdump. For instance, we use `-lookup` for addresses and `-find` for the accelerator tables. (https://llvm.org/docs/CommandGuide/llvm-dwarfdump.html) 

Would you mind using the same names here? 

Comment at: tools/lldb-test/lldb-test.cpp:244
+                             List);
+  if (List.GetSize() == 0) {
+    return make_error<StringError>("Context search didn't find a match.",

Comment at: tools/lldb-test/lldb-test.cpp:260
+    if (!RE.IsValid()) {
+      errs() << "ERROR: `" << Name << "` is not a valid regular expression.\n";
+      return;
You can use `WithColor::error()` from Support :-) 

Comment at: tools/lldb-test/lldb-test.cpp:303
+  SymbolContext SC;
+  DenseSet<SymbolFile *> SearchedFiles;
+  TypeMap Map;
This feels a bit C89-like; personally I prefer to initialize my variables as close to their use as possible. 


More information about the lldb-commits mailing list