[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)

[via cfe-commits]

================
@@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1,
 
   Value *MergedVal = nullptr;
   if (auto *RecordVal1 = dyn_cast<RecordValue>(&Val1)) {
-    [[maybe_unused]] auto *RecordVal2 = cast<RecordValue>(&Val2);
-
-    // Values to be merged are always associated with the same location in
-    // `LocToVal`. The location stored in `RecordVal` should therefore also
-    // be the same.
-    assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-    // `RecordVal1` and `RecordVal2` may have different properties associated
-    // with them. Create a new `RecordValue` without any properties so that we
-    // soundly approximate both values. If a particular analysis needs to merge
-    // properties, it should do so in `DataflowAnalysis::merge()`.
-    MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc());
+    auto *RecordVal2 = cast<RecordValue>(&Val2);
+
+    if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+      // `RecordVal1` and `RecordVal2` may have different properties associated
+      // with them. Create a new `RecordValue` without any properties so that we
+      // soundly approximate both values. If a particular analysis needs to
+      // merge properties, it should do so in `DataflowAnalysis::merge()`.
----------------
martinboehme wrote:

I see what you're saying. However, if I put this part of the comment ("If a particular analysis needs to merge properties...") above the if statement, it would read a bit strangely -- because in the case where the locations aren't the same, it's clear we need to create a new `RecordValue` anyway, and this will of course remove any properties.

What I'm trying to explain here is that we need to create a new `RecordValue` even if the location is the same, because the properties may be different -- and that therefore the analysis needs to merge properties even in this case. Does that make sense?

Open to suggestions on how I could clarify this.

https://github.com/llvm/llvm-project/pull/65319