C++: Respond to review comments:

- ArgumentNode is now abstract
- PrimaryArgumentNode is now an OperandNode.
- ArgumentIndirectionNode is now merged into SideEffectArgumentNode.

Author: MathiasVP

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -5,23 +5,15 @@ private import DataFlowDispatch
 
 /**
  * A data flow node that occurs as the argument of a call and is passed as-is
- * to the callable. Instance arguments (`this` pointer) are also included.
+ * to the callable. Instance arguments (`this` pointer) and read side effects
+ * on parameters are also included.
  */
-class ArgumentNode extends Node {
-  ArgumentNode() {
-    // To avoid making this class abstract, we enumerate its values here.
-    this instanceof PrimaryArgumentNode or
-    this instanceof SideEffectArgumentNode
-  }
-
+abstract class ArgumentNode extends OperandNode {
   /**
    * Holds if this argument occurs at the given position in the given call.
    * The instance argument is considered to have index `-1`.
    */
-  predicate argumentOf(DataFlowCall call, int pos) {
-    this.(PrimaryArgumentNode).argumentOf(call, pos) or
-    this.(SideEffectArgumentNode).argumentOf(call, pos)
-  }
+  abstract predicate argumentOf(DataFlowCall call, int pos);
 
   /** Gets the call in which this node is an argument. */
   DataFlowCall getCall() { this.argumentOf(result, _) }
@@ -31,44 +23,34 @@ class ArgumentNode extends Node {
  * A data flow node that occurs as the argument to a call, or an
  * implicit `this` pointer argument.
  */
-private class PrimaryArgumentNode extends InstructionNode {
-  PrimaryArgumentNode() { exists(CallInstruction call | instr = call.getAnArgument()) }
+private class PrimaryArgumentNode extends ArgumentNode {
+  override ArgumentOperand op;
 
-  /**
-   * Holds if this argument occurs at the given position in the given call.
-   * The instance argument is considered to have index `-1`.
-   */
-  predicate argumentOf(DataFlowCall call, int pos) {
-    instr = call.getPositionalArgument(pos)
+  PrimaryArgumentNode() { exists(CallInstruction call | op = call.getAnArgumentOperand()) }
+
+  override predicate argumentOf(DataFlowCall call, int pos) {
+    op = call.getPositionalArgumentOperand(pos)
     or
-    instr = call.getThisArgument() and pos = -1
+    op = call.getThisArgumentOperand() and pos = -1
   }
 }
 
 /**
  * A data flow node representing the read side effect of a call on a
  * specific parameter.
  */
-private class SideEffectArgumentNode extends OperandNode {
+private class SideEffectArgumentNode extends ArgumentNode {
   override SideEffectOperand op;
   ReadSideEffectInstruction read;
 
-  SideEffectArgumentNode() {
-    exists(CallInstruction call |
-      read.getPrimaryInstruction() = call and
-      op = read.getSideEffectOperand()
-    )
-  }
+  SideEffectArgumentNode() { op = read.getSideEffectOperand() }
 
-  /**
-   * Holds if this argument occurs at the given position in the given call.
-   * See `getArgumentPosOfSideEffect` for a describing of how read side effects
-   * are assigned an argument position.
-   */
-  predicate argumentOf(DataFlowCall call, int pos) {
+  override predicate argumentOf(DataFlowCall call, int pos) {
     read.getPrimaryInstruction() = call and
     pos = getArgumentPosOfSideEffect(read.getIndex())
   }
+
+  override string toString() { result = "Argument " + read.getIndex() + " indirection" }
 }
 
 private newtype TReturnKind =

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -505,21 +505,6 @@ class DefinitionByReferenceNode extends InstructionNode {
   }
 }
 
-/**
- * A node representing the memory pointed to by a function argument.
- *
- * This class exists only in order to override `toString`, which would
- * otherwise be the default implementation inherited from `OperandNode`.
- */
-private class ArgumentIndirectionNode extends OperandNode {
-  override SideEffectOperand op;
-  ReadSideEffectInstruction read;
-
-  ArgumentIndirectionNode() { read.getSideEffectOperand() = op }
-
-  override string toString() { result = "Argument " + read.getIndex() + " indirection" }
-}
-
 /**
  * A `Node` corresponding to a variable in the program, as opposed to the
  * value of that variable at some particular point. This can be used for

cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected

@@ -1,7 +1,7 @@
 edges
 | A.cpp:55:5:55:5 | set output argument [c] | A.cpp:56:13:56:15 | call to get |
 | A.cpp:55:12:55:19 | (C *)... | A.cpp:55:5:55:5 | set output argument [c] |
-| A.cpp:55:12:55:19 | new | A.cpp:55:12:55:19 | (C *)... |
+| A.cpp:55:12:55:19 | new | A.cpp:55:5:55:5 | set output argument [c] |
 | A.cpp:57:11:57:24 | B output argument [c] | A.cpp:57:28:57:30 | call to get |
 | A.cpp:57:17:57:23 | new | A.cpp:57:11:57:24 | B output argument [c] |
 | A.cpp:98:12:98:18 | new | A.cpp:100:5:100:13 | Store |
@@ -19,12 +19,11 @@ edges
 | A.cpp:143:7:143:31 | Chi [b] | A.cpp:151:12:151:24 | D output argument [b] |
 | A.cpp:143:7:143:31 | Store | A.cpp:143:7:143:31 | Chi [b] |
 | A.cpp:143:25:143:31 | new | A.cpp:143:7:143:31 | Store |
-| A.cpp:150:12:150:18 | new | A.cpp:151:18:151:18 | b |
+| A.cpp:150:12:150:18 | new | A.cpp:151:12:151:24 | D output argument [b] |
 | A.cpp:151:12:151:24 | Chi [b] | A.cpp:152:13:152:13 | b |
 | A.cpp:151:12:151:24 | D output argument [b] | A.cpp:151:12:151:24 | Chi [b] |
 | A.cpp:151:18:151:18 | Chi [c] | A.cpp:154:13:154:13 | c |
 | A.cpp:151:18:151:18 | D output argument [c] | A.cpp:151:18:151:18 | Chi [c] |
-| A.cpp:151:18:151:18 | b | A.cpp:151:12:151:24 | D output argument [b] |
 | C.cpp:18:12:18:18 | C output argument [s1] | C.cpp:27:8:27:11 | *#this [s1] |
 | C.cpp:18:12:18:18 | C output argument [s3] | C.cpp:27:8:27:11 | *#this [s3] |
 | C.cpp:22:12:22:21 | Chi [s1] | C.cpp:24:5:24:25 | Chi [s1] |
@@ -229,7 +228,6 @@ nodes
 | A.cpp:151:12:151:24 | D output argument [b] | semmle.label | D output argument [b] |
 | A.cpp:151:18:151:18 | Chi [c] | semmle.label | Chi [c] |
 | A.cpp:151:18:151:18 | D output argument [c] | semmle.label | D output argument [c] |
-| A.cpp:151:18:151:18 | b | semmle.label | b |
 | A.cpp:152:13:152:13 | b | semmle.label | b |
 | A.cpp:154:13:154:13 | c | semmle.label | c |
 | C.cpp:18:12:18:18 | C output argument [s1] | semmle.label | C output argument [s1] |