C/C++ overlay: Address review comments

Split the discard predicate into two: one for single-location elements and one for multi-location elements.

Author: IdrissRio

cpp/ql/lib/semmle/code/cpp/internal/Overlay.qll

@@ -16,58 +16,56 @@ private string getLocationFilePath(@location_default loc) {
 }
 
 /**
- * An element with a single location. Discard if in a changed file.
+ * Gets the file path for an element with a single location.
  */
 overlay[local]
-abstract private class Discardable extends @element {
-  abstract string getFilePath();
+private string getSingleLocationFilePath(@element e) {
+  // @var_decl has a direct location in the var_decls relation
+  exists(@location_default loc | var_decls(e, _, _, _, loc) |
+    result = getLocationFilePath(loc)
+  )
+  //TODO: add other kinds of elements with single locations
+}
 
-  predicate existsInBase() { not isOverlay() }
+/**
+ * Gets the file path for an element with potentially multiple locations.
+ */
+overlay[local]
+private string getMultiLocationFilePath(@element e) {
+  // @variable gets its location(s) from its @var_decl(s)
+  exists(@var_decl vd, @location_default loc | var_decls(vd, e, _, _, loc) |
+    result = getLocationFilePath(loc)
+  )
+  //TODO: add other kinds of elements with multiple locations
+}
 
-  string toString() { none() }
+/** Holds if `e` exists in the base variant. */
+overlay[local]
+private predicate existsInBase(@element e) {
+  not isOverlay() and
+  (exists(getSingleLocationFilePath(e)) or exists(getMultiLocationFilePath(e)))
 }
 
+/**
+ * Discard an element with a single location if it is in a changed file.
+ */
 overlay[discard_entity]
-private predicate discardable(@element e) {
-  e = any(Discardable d | d.existsInBase() and overlayChangedFiles(d.getFilePath()))
+private predicate discardSingleLocationElement(@element e) {
+  existsInBase(e) and
+  overlayChangedFiles(getSingleLocationFilePath(e))
 }
 
 /**
- * An element with potentially multiple locations, e.g., variables, functions and types.
- * Discard only if all locations are in changed files.
+ * Discard an element with multiple locations only if all its locations are in changed files.
  */
-overlay[local]
-abstract private class MultiDiscardable extends @element {
-  abstract string getFilePath();
+overlay[discard_entity]
+private predicate discardMultiLocationElement(@element e) {
+  existsInBase(e) and
+  exists(getMultiLocationFilePath(e)) and
+  forall(string path | path = getMultiLocationFilePath(e) | overlayChangedFiles(path))
+}
 
-  predicate existsInBase() { not isOverlay() }
 
-  string toString() { none() }
-}
 
-overlay[discard_entity]
-private predicate multiDiscardable(@element e) {
-  e =
-    any(MultiDiscardable d |
-      d.existsInBase() and
-      forall(string path | path = d.getFilePath() | overlayChangedFiles(path))
-    )
-}
 
-overlay[local]
-private class DiscardableVarDecl extends Discardable instanceof @var_decl {
-  override string getFilePath() {
-    exists(@location_default loc | var_decls(this, _, _, _, loc) |
-      result = getLocationFilePath(loc)
-    )
-  }
-}
 
-overlay[local]
-private class DiscardableVariable extends MultiDiscardable instanceof @variable {
-  override string getFilePath() {
-    exists(@var_decl vd, @location_default loc | var_decls(vd, this, _, _, loc) |
-      result = getLocationFilePath(loc)
-    )
-  }
-}