[DynInst_API:] [dyninst/dyninst] a36207: Make Expression the base of the InstructionAPI AST...

[Tim Haines <noreply@xxxxxxxxxx>]

  Branch: refs/heads/master
  Home:   https://github.com/dyninst/dyninst
  Commit: a36207d5bf084082c412b683d98b638649a66622
      https://github.com/dyninst/dyninst/commit/a36207d5bf084082c412b683d98b638649a66622
  Author: Tim Haines <thaines.astro@xxxxxxxxx>
  Date:   2025-04-21 (Mon, 21 Apr 2025)

  Changed paths:
    M dataflowAPI/src/Absloc.C
    M dataflowAPI/src/AbslocInterface.C
    M dyninstAPI/src/StackMod/StackAccess.C
    M instructionAPI/doc/2-Abstractions.tex
    M instructionAPI/doc/3-API.tex
    M instructionAPI/doc/API/BinaryFunction.tex
    M instructionAPI/doc/API/Dereference.tex
    M instructionAPI/doc/API/Expression.tex
    M instructionAPI/doc/API/Immediate.tex
    R instructionAPI/doc/API/InstructionAST.tex
    M instructionAPI/doc/API/MultiRegisterAST.tex
    M instructionAPI/doc/API/RegisterAST.tex
    M instructionAPI/doc/API/TernaryAST.tex
    M instructionAPI/h/BinaryFunction.h
    M instructionAPI/h/Dereference.h
    M instructionAPI/h/Expression.h
    M instructionAPI/h/Immediate.h
    M instructionAPI/h/InstructionAST.h
    M instructionAPI/h/MultiRegister.h
    M instructionAPI/h/Register.h
    M instructionAPI/h/Ternary.h
    M instructionAPI/src/BinaryFunction.C
    M instructionAPI/src/Expression.C
    M instructionAPI/src/Immediate.C
    M instructionAPI/src/MultiRegister.C
    M instructionAPI/src/Operand.C
    M instructionAPI/src/Register.C
    M instructionAPI/src/Ternary.C
    M instructionAPI/src/full_inheritance_graph.dot
    M patchAPI/src/PatchBlock.C

  Log Message:
  -----------
  Make Expression the base of the InstructionAPI AST hierarchy (#1959)

The InstructionAST abstraction doesn't provide any functionality beyond
its immediate child, Expression. This is particularly evident by

    all uses of InstructionAST in Dyninst are immediately converted to
    an Expression

    the lackluster description provided in the docs:

The InstructionAST class is the base class for all nodes in the ASTs
used by the Operand class. It defines the necessary interfaces for
traversing and searching an abstract syntax tree representing an
operand.

But the real problem is with 'getChildren'. Because both

InstructionAST::getChildren(std::vectorInstructionAST::Ptr const&)=0

and

Expression::getChildren(std::vectorExpression::Ptr const&)=0

exist, then every AST type has to implement both which is redundant and
provides no logical separation between an InstructionAST and an
Expression. In C++, these are referred to as overloaded virtual
functions and are generally a sign of an error since the one from the
base class will be hidden (see -Woverloaded-virtual). The author
"overcame" this by making them pure virtual. That doesn't remove the
logical incorrectness, though.

InstructionAST should have implemented 'getChildren' (or even better,
'getChildNodes') and Expression should have implemented
'getSubexpressions'. Then all of this would have been separated in a
useful way.



To unsubscribe from these emails, change your notification settings at https://github.com/dyninst/dyninst/settings/notifications