[Lldb-commits] [PATCH] D61056: PostfixExpression: move DWARF generator out of NativePDB internals

Adrian McCarthy via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Apr 24 16:38:17 PDT 2019

amccarth added a comment.

Can the test deletions be a separate patch, or will they fail because of the other changes in this patch?  They feel like a good but separable step.

Comment at: include/lldb/Symbol/PostfixExpression.h:210
+  bool Visit(UnaryOpNode &unary, Node *&ref) override {
+    return Dispatch(unary.Operand());
You could leave `ref` unnamed here to avoid any "unused parameter" warnings.

Comment at: include/lldb/Symbol/PostfixExpression.h:216
+  /// nullptr, if unable to replace/resolve.
+  virtual Node *Replace(SymbolNode &symbol) = 0;
I'm confused why this takes `symbol` by (non-const) ref.  Is `symbol` modified in the process of figuring out what should replace it?

Oh, peeking ahead at the implementation, I see it can return the address of `symbol`.  I'm left wondering whether there's a less confusing API.

Comment at: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:46
+  Node *result = it->second;
+  Dispatch(result); // Recursively process the result.
+  return result;    // And return it.
If I understand correctly, we don't care about the return value of `Dispatch` because all that matters is whether `result` points to a valid `Node` or is just `nullptr`.  Right?

If so, then should `SymbolResolver ` derive from `Visitor<void>` rather than `Visitor<bool>`?



More information about the lldb-commits mailing list