PDBFPO: Improvements to the AST visitor

[labath]

Summary

This patch attempts to solve two issues made this code hard to follow
for me.

The first issue was that a lot of what these visitors do is mutate the
AST. The visitor pattern is not particularly good for that because by
the time you have performed the dynamic type dispatch, it's too late to
go back to the parent node, and change its pointer. The previous code
dealt with that relatively elegantly, but it still meant that one had to
perform manual type checks, which is what the visitor pattern is
supposed to avoid.

The second issue was not being able to return values from the Visit
functions, which meant that one had to store function results in member
variables (a common problem with visitor patterns).

Here, I try to solve both problems by making the visitor a CRTP
template. This allows one to parameterize the visitor based on the
return type and pass function results as function results. The mutation
is fascilitated by having each Visit function take two arguments -- a
reference to the object itself (with the correct dynamic type), and a
reference to the parent's pointer to this object.

Although this wasn't my explicit goal here, the fact that we're not
using virtual dispatch anymore (not possible due to return type
templatization) allows us to make the AST nodes trivially destructible,
which is a good thing, since we were not destroying them anyway.

[labath]

I find this version easier to follow than the old one, but that could be simply because I didn't write it. :P
So if you believe the old one is better, I am happy to just drop this and carry on with the old one.

[labath]

Fix a bug which meant we weren't replacing the root node correctly (I somehow
forgot to run tests before uploading).

[amccarth]

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.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

291–292

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.

[labath]

After trying to use this in new code, I realized that the CRTP is not really
needed for what I am trying to achieve here. The same can be achieved through
more standard virtual functions.

This updates the patch to use virtual functions, which may be 0.01% slower, but
that certainly does not matter here. I also rename the function arguments from
"node" to more descriptive names.

[amccarth]

I'm even happier. Thanks for giving the params more specific names.

[teemperor]

I believe this revision introduced a warning when compiling with Clang:

 98% [4004/4047] Building CXX object tool....dir/PdbFPOProgramToDWARFExpression.cpp.o
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<void>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:51: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<void>' requested here
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
                                                  ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:7: warning: '(anonymous namespace)::FPOProgramASTVisitorMergeDependent' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<bool>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:232:14: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<bool>' requested here
    : public FPOProgramASTVisitor<bool> {
             ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:231:7: warning: '(anonymous namespace)::FPOProgramASTVisitorResolveRegisterRefs' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorResolveRegisterRefs
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:310:7: warning: '(anonymous namespace)::FPOProgramASTVisitorDWARFCodegen' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
      ^
5 warnings generated.

[labath]

In D60410#1464214, @teemperor wrote:

I believe this revision introduced a warning when compiling with Clang:

 98% [4004/4047] Building CXX object tool....dir/PdbFPOProgramToDWARFExpression.cpp.o
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<void>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:51: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<void>' requested here
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
                                                  ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:7: warning: '(anonymous namespace)::FPOProgramASTVisitorMergeDependent' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<bool>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:232:14: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<bool>' requested here
    : public FPOProgramASTVisitor<bool> {
             ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:231:7: warning: '(anonymous namespace)::FPOProgramASTVisitorResolveRegisterRefs' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorResolveRegisterRefs
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:310:7: warning: '(anonymous namespace)::FPOProgramASTVisitorDWARFCodegen' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
      ^
5 warnings generated.

Thanks for the heads up. I'll fix that right away.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

 #include "llvm/DebugInfo/CodeView/EnumTables.h"
 #include "llvm/Support/Casting.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 
-class FPOProgramNode;
-class FPOProgramASTVisitor;
-
 class NodeAllocator {
 public:
   template <typename T, typename... Args> T *makeNode(Args &&... args) {
+    static_assert(std::is_trivially_destructible<T>::value,
+                  "This object will not be destroyed!");
     void *new_node_mem = m_alloc.Allocate(sizeof(T), alignof(T));
     return new (new_node_mem) T(std::forward<Args>(args)...);
   }
 
 private:
   llvm::BumpPtrAllocator m_alloc;
 };
 
 class FPOProgramNode {
 public:
   enum Kind {
     Symbol,
     Register,
     IntegerLiteral,
     BinaryOp,
     UnaryOp,
   };
 
 protected:
   FPOProgramNode(Kind kind) : m_token_kind(kind) {}
 
 public:
-  virtual ~FPOProgramNode() = default;
-  virtual void Accept(FPOProgramASTVisitor &visitor) = 0;
-
   Kind GetKind() const { return m_token_kind; }
 
 private:
   Kind m_token_kind;
 };
 
 class FPOProgramNodeSymbol: public FPOProgramNode {
 public:
   FPOProgramNodeSymbol(llvm::StringRef name)
       : FPOProgramNode(Symbol), m_name(name) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   llvm::StringRef GetName() const { return m_name; }
 
   static bool classof(const FPOProgramNode *node) {
     return node->GetKind() == Symbol;
   }
 
 private:
   llvm::StringRef m_name;
 };
 
 class FPOProgramNodeRegisterRef : public FPOProgramNode {
 public:
   FPOProgramNodeRegisterRef(uint32_t lldb_reg_num)
       : FPOProgramNode(Register), m_lldb_reg_num(lldb_reg_num) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
 
   static bool classof(const FPOProgramNode *node) {
     return node->GetKind() == Register;
   }
 
 private:
   uint32_t m_lldb_reg_num;
 };
 
 class FPOProgramNodeIntegerLiteral : public FPOProgramNode {
 public:
   FPOProgramNodeIntegerLiteral(uint32_t value)
       : FPOProgramNode(IntegerLiteral), m_value(value) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   uint32_t GetValue() const { return m_value; }
 
   static bool classof(const FPOProgramNode *node) {
     return node->GetKind() == IntegerLiteral;
   }
 
 private:
   uint32_t m_value;
 };
 
 class FPOProgramNodeBinaryOp : public FPOProgramNode {
 public:
   enum OpType {
     Plus,
     Minus,
     Align,
   };
 
   FPOProgramNodeBinaryOp(OpType op_type, FPOProgramNode &left,
                          FPOProgramNode &right)
       : FPOProgramNode(BinaryOp), m_op_type(op_type), m_left(&left),
         m_right(&right) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   OpType GetOpType() const { return m_op_type; }
 
   const FPOProgramNode *Left() const { return m_left; }
   FPOProgramNode *&Left() { return m_left; }
 
   const FPOProgramNode *Right() const { return m_right; }
   FPOProgramNode *&Right() { return m_right; }
 

 public:
   enum OpType {
     Deref,
   };
 
   FPOProgramNodeUnaryOp(OpType op_type, FPOProgramNode &operand)
       : FPOProgramNode(UnaryOp), m_op_type(op_type), m_operand(&operand) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   OpType GetOpType() const { return m_op_type; }
 
   const FPOProgramNode *Operand() const { return m_operand; }
   FPOProgramNode *&Operand() { return m_operand; }
 
   static bool classof(const FPOProgramNode *node) {
     return node->GetKind() == UnaryOp;
   }
 
 private:
   OpType m_op_type;
   FPOProgramNode *m_operand;
 };
 
+template <typename ResultT = void>
 class FPOProgramASTVisitor {
-public:
-  virtual ~FPOProgramASTVisitor() = default;
+protected:
+  virtual ResultT Visit(FPOProgramNodeBinaryOp &binary,
+                        FPOProgramNode *&ref) = 0;
+  virtual ResultT Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&ref) = 0;
+  virtual ResultT Visit(FPOProgramNodeRegisterRef &reg, FPOProgramNode *&) = 0;
+  virtual ResultT Visit(FPOProgramNodeIntegerLiteral &integer,
+                        FPOProgramNode *&) = 0;
+  virtual ResultT Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) = 0;
 
-  virtual void Visit(FPOProgramNodeSymbol &node) {}
-  virtual void Visit(FPOProgramNodeRegisterRef &node) {}
-  virtual void Visit(FPOProgramNodeIntegerLiteral &node) {}
-  virtual void Visit(FPOProgramNodeBinaryOp &node) {}
-  virtual void Visit(FPOProgramNodeUnaryOp &node) {}
-};
+  ResultT Dispatch(FPOProgramNode *&node) {
+    switch (node->GetKind()) {
+    case FPOProgramNode::Register:
+      return Visit(llvm::cast<FPOProgramNodeRegisterRef>(*node), node);
+    case FPOProgramNode::Symbol:
+      return Visit(llvm::cast<FPOProgramNodeSymbol>(*node), node);
 
-void FPOProgramNodeSymbol::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
+    case FPOProgramNode::IntegerLiteral:
+      return Visit(llvm::cast<FPOProgramNodeIntegerLiteral>(*node), node);
+    case FPOProgramNode::UnaryOp:
+      return Visit(llvm::cast<FPOProgramNodeUnaryOp>(*node), node);
+    case FPOProgramNode::BinaryOp:
+      return Visit(llvm::cast<FPOProgramNodeBinaryOp>(*node), node);
     }
-
-void FPOProgramNodeRegisterRef::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
+    llvm_unreachable("Fully covered switch!");
   }
 
-void FPOProgramNodeIntegerLiteral::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
+};
+
+class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
+public:
+  void Visit(FPOProgramNodeBinaryOp &binary, FPOProgramNode *&) override {
+    Dispatch(binary.Left());
+    Dispatch(binary.Right());
   }
 
-void FPOProgramNodeBinaryOp::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
+  void Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&) override {
+    Dispatch(unary.Operand());
   }
 
-void FPOProgramNodeUnaryOp::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
+  void Visit(FPOProgramNodeRegisterRef &, FPOProgramNode *&) override {}
+  void Visit(FPOProgramNodeIntegerLiteral &, FPOProgramNode *&) override {}
+  void Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) override;
+
+  static void Merge(const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
+                        &dependent_programs,
+                    FPOProgramNode *&ast) {
+    FPOProgramASTVisitorMergeDependent(dependent_programs).Dispatch(ast);
   }
 
-class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor {
-public:
+private:
   FPOProgramASTVisitorMergeDependent(
       const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
           &dependent_programs)
       : m_dependent_programs(dependent_programs) {}
 
-  void Merge(FPOProgramNode *&node_ref);
-
-private:
-  void Visit(FPOProgramNodeBinaryOp &node) override;
-  void Visit(FPOProgramNodeUnaryOp &node) override;
-
-  void TryReplace(FPOProgramNode *&node_ref) const;
-
-private:
   const llvm::DenseMap<llvm::StringRef, FPOProgramNode *> &m_dependent_programs;
 };
 
-void FPOProgramASTVisitorMergeDependent::Merge(FPOProgramNode *&node_ref) {
-  TryReplace(node_ref);
-  node_ref->Accept(*this);
-}
+void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeSymbol &symbol,
+                                               FPOProgramNode *&ref) {
+
+  auto it = m_dependent_programs.find(symbol.GetName());
+  if (it == m_dependent_programs.end())
+    return;
 
-void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeBinaryOp &node) {
-  Merge(node.Left());
-  Merge(node.Right());
+  ref = it->second;
+  Dispatch(ref);
 }
-void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeUnaryOp &node) {
-  Merge(node.Operand());
+
+class FPOProgramASTVisitorResolveRegisterRefs
+    : public FPOProgramASTVisitor<bool> {
+public:
+  static bool Resolve(const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
+                          &dependent_programs,
+                      llvm::Triple::ArchType arch_type, NodeAllocator &alloc,
+                      FPOProgramNode *&ast) {
+    return FPOProgramASTVisitorResolveRegisterRefs(dependent_programs,
+                                                   arch_type, alloc)
+        .Dispatch(ast);
   }
 
-void FPOProgramASTVisitorMergeDependent::TryReplace(
-    FPOProgramNode *&node_ref) const {
+  bool Visit(FPOProgramNodeBinaryOp &binary, FPOProgramNode *&) override {
+    return Dispatch(binary.Left()) && Dispatch(binary.Right());
+  }
 
-  while (auto *symbol = llvm::dyn_cast<FPOProgramNodeSymbol>(node_ref)) {
-    auto it = m_dependent_programs.find(symbol->GetName());
-    if (it == m_dependent_programs.end()) {
-      break;
-    }
+  bool Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&) override {
+    return Dispatch(unary.Operand());
 
-    node_ref = it->second;
+  bool Visit(FPOProgramNodeRegisterRef &, FPOProgramNode *&) override {
+    return true;
   }
+
+  bool Visit(FPOProgramNodeIntegerLiteral &, FPOProgramNode *&) override {
+    return true;
   }
 
-class FPOProgramASTVisitorResolveRegisterRefs : public FPOProgramASTVisitor {
-public:
+  bool Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) override;
+
+private:
   FPOProgramASTVisitorResolveRegisterRefs(
       const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
           &dependent_programs,
       llvm::Triple::ArchType arch_type, NodeAllocator &alloc)
       : m_dependent_programs(dependent_programs), m_arch_type(arch_type),
         m_alloc(alloc) {}
 
-  bool Resolve(FPOProgramNode *&program);
-
-private:
-  void Visit(FPOProgramNodeBinaryOp &node) override;
-  void Visit(FPOProgramNodeUnaryOp &node) override;
-
-  bool TryReplace(FPOProgramNode *&node_ref);
-
   const llvm::DenseMap<llvm::StringRef, FPOProgramNode *> &m_dependent_programs;
   llvm::Triple::ArchType m_arch_type;
   NodeAllocator &m_alloc;
-  bool m_no_error_flag = true;
 };
 
-bool FPOProgramASTVisitorResolveRegisterRefs::Resolve(FPOProgramNode *&program) {
-  if (!TryReplace(program))
-    return false;
-  program->Accept(*this);
-  return m_no_error_flag;
-}
-
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
   llvm::ArrayRef<llvm::EnumEntry<uint16_t>> register_names =
       llvm::codeview::getRegisterNames();
   auto it = llvm::find_if(
       register_names,
       [&reg_name](const llvm::EnumEntry<uint16_t> &register_entry) {
         return reg_name.compare_lower(register_entry.Name) == 0;
       });
 
   if (it == register_names.end())
     return LLDB_INVALID_REGNUM;
 
   auto reg_id = static_cast<llvm::codeview::RegisterId>(it->Value);
   return npdb::GetLLDBRegisterNumber(arch_type, reg_id);
 }
 
-bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
-    FPOProgramNode *&node_ref) {
-  auto *symbol = llvm::dyn_cast<FPOProgramNodeSymbol>(node_ref);
-  if (!symbol)
-    return true;
-
+bool FPOProgramASTVisitorResolveRegisterRefs::Visit(
+    FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) {
   // Look up register reference as lvalue in preceding assignments.
-  auto it = m_dependent_programs.find(symbol->GetName());
+  auto it = m_dependent_programs.find(symbol.GetName());
   if (it != m_dependent_programs.end()) {
     // Dependent programs are handled elsewhere.
     return true;
   }
 
   uint32_t reg_num =
-      ResolveLLDBRegisterNum(symbol->GetName().drop_front(1), m_arch_type);
+      ResolveLLDBRegisterNum(symbol.GetName().drop_front(1), m_arch_type);
 
   if (reg_num == LLDB_INVALID_REGNUM)
     return false;
 
-  node_ref = m_alloc.makeNode<FPOProgramNodeRegisterRef>(reg_num);
+  ref = m_alloc.makeNode<FPOProgramNodeRegisterRef>(reg_num);
   return true;
 }
 
-void FPOProgramASTVisitorResolveRegisterRefs::Visit(
-    FPOProgramNodeBinaryOp &node) {
-  m_no_error_flag = Resolve(node.Left()) && Resolve(node.Right());
+class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
+public:
+  static void Emit(Stream &stream, FPOProgramNode *&ast) {
+    FPOProgramASTVisitorDWARFCodegen(stream).Dispatch(ast);
   }
 
-void FPOProgramASTVisitorResolveRegisterRefs::Visit(
-    FPOProgramNodeUnaryOp &node) {
-  m_no_error_flag = Resolve(node.Operand());
+  void Visit(FPOProgramNodeRegisterRef &reg, FPOProgramNode *&);
+  void Visit(FPOProgramNodeBinaryOp &binary, FPOProgramNode *&);
+  void Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&);
+  void Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&) {
+    llvm_unreachable("Symbols should have been resolved by now!");
   }
-
-class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor {
-public:
-  FPOProgramASTVisitorDWARFCodegen(Stream &stream) : m_out_stream(stream) {}
-
-  void Emit(FPOProgramNode &program);
+  void Visit(FPOProgramNodeIntegerLiteral &integer, FPOProgramNode *&);
 
 private:
-  void Visit(FPOProgramNodeRegisterRef &node) override;
-  void Visit(FPOProgramNodeIntegerLiteral &node) override;
-  void Visit(FPOProgramNodeBinaryOp &node) override;
-  void Visit(FPOProgramNodeUnaryOp &node) override;
+  FPOProgramASTVisitorDWARFCodegen(Stream &stream) : m_out_stream(stream) {}
 
-private:
   Stream &m_out_stream;
 };
 
-void FPOProgramASTVisitorDWARFCodegen::Emit(FPOProgramNode &program) {
-  program.Accept(*this);
-}
+void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeRegisterRef &reg,
+                                             FPOProgramNode *&) {
 
-void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeRegisterRef &node) {
-
-  uint32_t reg_num = node.GetLLDBRegNum();
+  uint32_t reg_num = reg.GetLLDBRegNum();
   lldbassert(reg_num != LLDB_INVALID_REGNUM);
 
   if (reg_num > 31) {
     m_out_stream.PutHex8(DW_OP_bregx);
     m_out_stream.PutULEB128(reg_num);
   } else
     m_out_stream.PutHex8(DW_OP_breg0 + reg_num);
 
   m_out_stream.PutSLEB128(0);
 }
 
 void FPOProgramASTVisitorDWARFCodegen::Visit(
-    FPOProgramNodeIntegerLiteral &node) {
-  uint32_t value = node.GetValue();
+    FPOProgramNodeIntegerLiteral &integer, FPOProgramNode *&) {
+  uint32_t value = integer.GetValue();
   m_out_stream.PutHex8(DW_OP_constu);
   m_out_stream.PutULEB128(value);
 }
 
-void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeBinaryOp &node) {
-
-  Emit(*node.Left());
-  Emit(*node.Right());
+void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeBinaryOp &binary,
+                                             FPOProgramNode *&) {
+  Dispatch(binary.Left());
+  Dispatch(binary.Right());
 
-  switch (node.GetOpType()) {
+  switch (binary.GetOpType()) {
   case FPOProgramNodeBinaryOp::Plus:
     m_out_stream.PutHex8(DW_OP_plus);
     // NOTE: can be optimized by using DW_OP_plus_uconst opcpode
     //       if right child node is constant value
     break;
   case FPOProgramNodeBinaryOp::Minus:
     m_out_stream.PutHex8(DW_OP_minus);
     break;
   case FPOProgramNodeBinaryOp::Align:
     // emit align operator a @ b as
     // a & ~(b - 1)
     // NOTE: implicitly assuming that b is power of 2
     m_out_stream.PutHex8(DW_OP_lit1);
     m_out_stream.PutHex8(DW_OP_minus);
     m_out_stream.PutHex8(DW_OP_not);
 
     m_out_stream.PutHex8(DW_OP_and);
     break;
   }
 }
 
-void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeUnaryOp &node) {
-  Emit(*node.Operand());
+void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeUnaryOp &unary,
+                                             FPOProgramNode *&) {
+  Dispatch(unary.Operand());
 
-  switch (node.GetOpType()) {
+  switch (unary.GetOpType()) {
   case FPOProgramNodeUnaryOp::Deref:
     m_out_stream.PutHex8(DW_OP_deref);
     break;
   }
 }
 
 } // namespace
 
while (true) {
     if (!ParseFPOSingleAssignmentProgram(assignment_program, alloc, lvalue_name,
                                          rvalue_ast)) {
       return nullptr;
     }
 
     lldbassert(rvalue_ast);
 
     // check & resolve assignment program
-    FPOProgramASTVisitorResolveRegisterRefs resolver(dependent_programs,
-                                                     arch_type, alloc);
-    if (!resolver.Resolve(rvalue_ast)) {
+    if (!FPOProgramASTVisitorResolveRegisterRefs::Resolve(
+            dependent_programs, arch_type, alloc, rvalue_ast))
       return nullptr;
-    }
 
     if (lvalue_name == register_name) {
       // found target assignment program - no need to parse further
 
       // emplace valid dependent subtrees to make target assignment independent
       // from predecessors
-      FPOProgramASTVisitorMergeDependent merger(dependent_programs);
-      merger.Merge(rvalue_ast);
+      FPOProgramASTVisitorMergeDependent::Merge(dependent_programs, rvalue_ast);
 
       return rvalue_ast;
     }
 
     dependent_programs[lvalue_name] = rvalue_ast;
     cur = assign_index + 1;
   }
 
   return nullptr;
 }
 
 bool lldb_private::npdb::TranslateFPOProgramToDWARFExpression(
     llvm::StringRef program, llvm::StringRef register_name,
     llvm::Triple::ArchType arch_type, Stream &stream) {
   NodeAllocator node_alloc;
   FPOProgramNode *target_program =
       ParseFPOProgram(program, register_name, arch_type, node_alloc);
   if (target_program == nullptr) {
     return false;
   }
 
-  FPOProgramASTVisitorDWARFCodegen codegen(stream);
-  codegen.Emit(*target_program);
+  FPOProgramASTVisitorDWARFCodegen::Emit(stream, target_program);
   return true;
 }