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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth marked 2 inline comments as done.
amccarth added a comment.
This revision is now accepted and ready to land.

Clever (hopefully not too clever)!  Not having to derive a special class from Visitor is really nice.

LGTM.



================
Comment at: include/lldb/Symbol/PostfixExpression.h:216
+  /// nullptr, if unable to replace/resolve.
+  virtual Node *Replace(SymbolNode &symbol) = 0;
+};
----------------
labath wrote:
> 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.
Well, it is simpler, but we still have that non-const ref to symbol, which is what originally threw me.  I don't see a way to get around that, and I like the new solution better than having to derive your own visitor.


================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:46
+  Node *result = it->second;
+  Dispatch(result); // Recursively process the result.
+  return result;    // And return it.
----------------
labath wrote:
> 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).
Oh yes.  I was looking at the Dispatch calls in the DWARF CodeGen class by mistake.  Those are Visitor<void>.  Perfect.


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

https://reviews.llvm.org/D61056