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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:46
+  if (AllowNullOp && !Op) {
+    Result.push_back(nullptr);
+    return Result;
----------------
StephenTozer wrote:
> 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?
I'm sorry for having brought this up — it's always the person who dares touching something who then gets tasked with fixing up our technical debt :-)

> 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?

The iterator is more elegant, but really the SmallVector affords the exact same UX, which is being able to write `for (auto i : fn())`. So I think I'll leave this up to you. The iterator will perform slightly better if we have early exists, so that good, but we can also decide to not fix this now and move on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88230