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

[Adrian McCarthy via Phabricator via lldb-commits]

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>`?


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

https://reviews.llvm.org/D61056