[PATCH] D72589: Add GDB pretty printers for llvm::ilist, llvm::simple_ilist, and llvm::ilist_node.

[Christian Sigg via Phabricator via llvm-commits]

csigg marked 5 inline comments as done.
csigg added inline comments.


================
Comment at: debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp:38
+}();
+auto SimpleIlist = [&]() {
+  llvm::simple_ilist<IlistNode, SimpleIlistTag> Result;
----------------
csigg wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > (looks like this capture should be removed, according to the lint check)
> > Ping on this - please remove the capture (remove the "&" in the "[&]") since it doesn't capture anything. (global variables are accessible directly, without capturing them)
> Indeed, don't need to capture globals.
Sorry, I think the update was in flight while you were reviewing.


================
Comment at: llvm/utils/gdb-scripts/prettyprinters.py:399
+    children = [('sentinel', 'yes')] + children
+  return make_printer(children=children)
+
----------------
dblaikie wrote:
> Do you think the make_printer usage is sufficiently more readable than having a standalone pretty printer type (like IlistPrinter below)? 
> 
> I think there might be some value in just always writing these as standalone pretty printer classes rather than having some of them as factories and some as classes.
> 
I changed it to a class, which makes sense because initialization should not fail.

But for what it's worth, I don't really see the 'some value'. I think it was perfectly readable and concise before.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72589/new/

https://reviews.llvm.org/D72589