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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
