C++: Address review comments

Author: calumgrant

cpp/ql/lib/semmle/code/cpp/Function.qll

@@ -505,8 +505,11 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
    * Holds if this function has extraction errors that create an `ErrorExpr`.
    */
   predicate hasErrors() {
-    // Exclude allocator call arguments because they are are always extracted as `ErrorExpr`.
-    exists(ErrorExpr e | e.getEnclosingFunction() = this and not e.isFirstAllocatorCallArgument())
+    exists(ErrorExpr e |
+      e.getEnclosingFunction() = this and
+      // Exclude the first allocator call argument because it is always extracted as `ErrorExpr`.
+      not exists(NewOrNewArrayExpr new | e = new.getAllocatorCall().getArgument(0))
+    )
   }
 }
 

cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll

@@ -744,13 +744,6 @@ class ErrorExpr extends Expr, @errorexpr {
   override string toString() { result = "<error expr>" }
 
   override string getAPrimaryQlClass() { result = "ErrorExpr" }
-
-  /**
-   * Holds if this error expression is the first argument to a `new` allocation call.
-   */
-  predicate isFirstAllocatorCallArgument() {
-    this = any(NewOrNewArrayExpr new).getAllocatorCall().getArgument(0)
-  }
 }
 
 /**

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -151,7 +151,7 @@ private predicate ignoreExprOnly(Expr expr) {
   // The extractor deliberately emits an `ErrorExpr` as the first argument to
   // the allocator call, if any, of a `NewOrNewArrayExpr`. That `ErrorExpr`
   // should not be translated.
-  expr.(ErrorExpr).isFirstAllocatorCallArgument()
+  exists(NewOrNewArrayExpr new | expr = new.getAllocatorCall().getArgument(0))
   or
   not translateFunction(getEnclosingFunction(expr)) and
   not Raw::varHasIRFunc(getEnclosingVariable(expr))

cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql

@@ -65,6 +65,7 @@ predicate isSinkImpl(Instruction sink, VariableAccess va) {
   exists(LoadInstruction load |
     va = load.getUnconvertedResultExpression() and
     not va = commonException() and
+    not va.getTarget().(LocalVariable).getFunction().hasErrors() and
     sink = load.getSourceValue()
   )
 }
@@ -89,6 +90,5 @@ from
 where
   conf.hasFlowPath(source, sink) and
   isSinkImpl(sink.getInstruction(), va) and
-  v = va.getTarget() and
-  not v.getFunction().hasErrors()
+  v = va.getTarget()
 select va, source, sink, "The variable $@ may not be initialized at this access.", v, v.getName()

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected

@@ -1,6 +1,5 @@
 edges
 nodes
-| errors.cpp:4:7:4:7 | definition of x | semmle.label | definition of x |
 | errors.cpp:13:7:13:7 | definition of x | semmle.label | definition of x |
 | test.cpp:11:6:11:8 | definition of foo | semmle.label | definition of foo |
 | test.cpp:111:6:111:8 | definition of foo | semmle.label | definition of foo |