[PATCH] D88230: [DebugInfo] Support multiple location operands via DIArgList in DbgVariableIntrinsics

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:46
+  if (AllowNullOp && !Op) {
+    Result.push_back(nullptr);
+    return Result;
----------------
aprantl wrote:
> Maybe this isn't such a big deal, but this function has a weird UI now — returning a SmallVector with nullptrs... I wonder if we should instead return a small iterator object? How is this used?
I agree that the UI to this function is weird. Frankly, I don't think this should ever have been returning `nullptr` in the first place - only a malformed debug intrinsic should have a first argument which isn't a valid `MetadataAsValue` (this is confirmed by the verifier). I've kept it here only because I was avoiding changing the behaviour of the existing code that uses the `nullptr` return value. If there must be an "error" response, it also shouldn't be the empty list, because we may want to use that for constants such as:

`llvm.dbg.value(!DIArgList(), !"x", !DIExpression(DW_OP_uconst, 5, DW_OP_stack_value))`

Putting the `nullptr` issue aside, it might still be better to return an iterator for this that casts individual arguments to `Value*` with each iteration. I returned a `SmallVector` here for simplicity's sake, but for general use it might be easier to define a custom iterator for this class. Do you think it'd be useful in that case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88230