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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lit/SymbolFile/DWARF/basic-function-lookup.cpp:23
+void foo() {}
+// BASE-DAG: name = "foo()", mangled = "_Z3foov"
+// FULL-DAG: name = "foo()", mangled = "_Z3foov"
----------------
zturner wrote:
> I personally find it a little confusing to have the check lines interspersed with the source code here as we do in this file.  I think it's easier to read if all check labels are grouped together so that you get a general idea of what the output of the tool looks like.  I don't feel too strongly though, so up to you.
I agree it looks better when it's grouped. I did it this way originally, because the first tests I was writing were the variable & type tests, and there I used ``@LINE`` thingies to match the declarations (as the output stands now, the declaration line is the only sufficiently deterministic property I can use to distinguish two variables with the same name). I have regrouped the checks which don't depend on the line numbers.


================
Comment at: tools/lldb-test/lldb-test.cpp:86
+};
+static cl::opt<LookupType> Lookup(
+    "lookup", cl::desc("Choose lookup type:"),
----------------
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`, ...)


================
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.",
----------------
JDevlieghere wrote:
> `Empty()`?
There isn't such a function, but having it sounds like a good idea, so I added one. :)


https://reviews.llvm.org/D46318