[PATCH] D37877: Update getMergedLocation to check the instruction type and merge properly.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

LGTM, I would add one extra line to the comment of applyMergedLocation (see inline).
Thanks!



================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1426
+  /// treated the same as other instructions. Otherwise, use
+  /// Instruction::applyMergedLocation instead.
   static const DILocation *getMergedLocation(const DILocation *LocA,
----------------
I think if we spell this as `\p applyMergedLocation` it gets turned into a link by Doxygen, might be worth trying.


================
Comment at: include/llvm/IR/Instruction.h:391
+  ///     applications, thus the N-way merging should be in code path.
+  void applyMergedLocation(const DILocation *LocA, const DILocation *LocB);
+
----------------
danielcdh wrote:
> aprantl wrote:
> > Should we document what happens to the debug location already attached to `this`? Or - looking at the use-cases - should it assert that the debugloc is empty?
> Not sure if I understand this. this->debugloc will be either empty, or locA or locB. But it will be overwritten anyway, why do we care about its value?
I was thinking of adding an extra line like to make the behavior clear to the casual reader:
`/// The DebugLoc attached to this instruction will be overwritten by the merged DebugLoc.`


https://reviews.llvm.org/D37877