[PATCH] D114355: [DebugInfo] ValueMapper impl for DIArgList should respect RF_IgnoreMissingLocals

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/unittests/Transforms/Utils/ValueMapperTest.cpp:323
+  EXPECT_EQ(N1AV, ValueMapper(VM).mapValue(*MAV1));
+  EXPECT_EQ(MAV1, ValueMapper(VM, RF_IgnoreMissingLocals).mapValue(*MAV1));
+  EXPECT_FALSE(VM.count(MAV0));
----------------
StephenTozer wrote:
> jmorse wrote:
> > (Line I'm referring to in main comment)
> I believe the "we shouldn't reference the unmapped local" comment is in reference to the non-IgnoreMissingLocals case specifically. The return value of nullptr for the IgnoreMissingLocals case is, as far as I can tell, equivalent to returning the unmapped local - this is the only site at which we may call this on a LocalAsMetadata/DIArgList: 
> 
> ```
>     Value *V = mapValue(Op);
>     // If we aren't ignoring missing entries, assert that something happened.
>     if (V)
>       Op = V;
>     else
>       assert((Flags & RF_IgnoreMissingLocals) &&
>              "Referenced value not in value map!");
> ```
> 
Just to expand on this answer - returning `nullptr` if no mapping can be found is meant to be the standard for mapValue, so that the caller can decide what to do about that; the specific difficulty with DIArgList is that there may be //some// elements with a mapping and some without. In this case, this patch makes the executive decision to keep the unmapped value in the returned DIArgList, rather than replacing it with a nullptr and expecting the caller to manually check for a DIArgList with nullptr elements. It may be worth adding a comment to this patch to explain that, however!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114355/new/

https://reviews.llvm.org/D114355