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