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