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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added a comment.

In D61056#1477914 <https://reviews.llvm.org/D61056#1477914>, @amccarth wrote:

> 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.


Nope, the tests still pass, they just felt superfluous. I'll remove them in a separate patch.



================
Comment at: include/lldb/Symbol/PostfixExpression.h:216
+  /// nullptr, if unable to replace/resolve.
+  virtual Node *Replace(SymbolNode &symbol) = 0;
+};
----------------
amccarth wrote:
> 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.
Yeah, I too had the feeling that there ought to be a simpler way to do this, but I couldn't figure out how.... until now...

Check out the new version of the patch. Now, instead of having to subclass something one can implement a trivial search-and-replace operation by just passing a callback to the function. (There is subclassing going on under the hood, but the user is unaware of that).

In fact, it made things so simple, I just decided to merge the two-pass algorithm in NativePDB into a single function.


================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:46
+  Node *result = it->second;
+  Dispatch(result); // Recursively process the result.
+  return result;    // And return it.
----------------
amccarth wrote:
> 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>`?
Not exactly. The fact whether is the replacement function returned nullptr is then propagated up the Dispatch function, and eventually makes its way out as the result of the entire replacement operation (so, yes, the bool is needed here).


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

https://reviews.llvm.org/D61056