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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: tools/lldb-test/lldb-test.cpp:86
+};
+static cl::opt<LookupType> Lookup(
+    "lookup", cl::desc("Choose lookup type:"),
----------------
labath wrote:
> JDevlieghere wrote:
> > 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? 
> Hmm... I agree we should avoid confusion where possible. So using `lookup` is out.
> How about renaming it to `find`. It's nice that it matches the underlying lldb functions, but it is also a bit inconsistent because llvm-dwarfdump uses it to search through the accelerator tables. However we don't have the distinction between searching using accelerator tables and not, and I don't think we will need it (the idea here was to make this search the same way that lldb would do).
> 
> The alternative is to rename this argument to something completely different (`--search`, `--name-type`, ...)
Alright, `find` sounds okay. 


================
Comment at: tools/lldb-test/lldb-test.cpp:260
+    if (!RE.IsValid()) {
+      errs() << "ERROR: `" << Name << "` is not a valid regular expression.\n";
+      return;
----------------
JDevlieghere wrote:
> You can use `WithColor::error()` from Support :-) 
I probably should've noticed this earlier, but since we return after all these errors maybe I'd be nice to have the function return an error? Doesn't have to be in this diff though. 


================
Comment at: tools/lldb-test/lldb-test.cpp:463
 
   DebuggerLifetime->Terminate();
   return 0;
----------------
If we can wrap this in a RAII object we could have this tool return a non-zero exit code in case of an error. 


https://reviews.llvm.org/D46318