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

[Jonas Devlieghere via Phabricator via lldb-commits]

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.",
----------------
`Empty()`?


================
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. 


https://reviews.llvm.org/D46318