[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

Mon Apr 8 16:25:45 PDT 2019

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

Overall, I'm ambivalent.

The patch description makes a good case for the change.  I find the visitor pattern hard to follow (partly because the `visit` and `accept` naming is not intuitive for me).  I find CRTP more understandable.  And you make a good argument about the visitor pattern not being right for a mutator.  So I'm inclined to like this change

But in practice, I don't find the change any easier to read, and the switch on the type _seems_ like a code smell even though it's justified here.

I'm OK with it if nobody else objects.

================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:284

-bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
-    FPOProgramNode *&node_ref) {
-  auto *symbol = llvm::dyn_cast<FPOProgramNodeSymbol>(node_ref);
-  if (!symbol)
-    return true;
-
+bool FPOProgramASTVisitorResolveRegisterRefs::Visit(FPOProgramNodeSymbol &node,
+                                                    FPOProgramNode *&ref) {
----------------
I actually preferred `symbol` for this parameter name.  `node` is very generic.  At this point, we know the node is a symbol, so using the more specific name would be more consistent.

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

https://reviews.llvm.org/D60410