[PATCH] D29833: Improve the API of DILocation::getMergedLocation()

[David Blaikie via llvm-commits]

On Wed, Feb 22, 2017 at 4:21 PM Adrian Prantl via Phabricator <
reviews at reviews.llvm.org> wrote:

> aprantl added a comment.
>
> [apologies for iterating so slowly at the moment]
>
> In https://reviews.llvm.org/D29833#682595, @twoh wrote:
>
> > Personally I prefer the existing API, because it doesn't allow the
> transient state before merging. For example, in
> lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp in this patch,
> DebugLoc of NewSI is not actually valid after line 1430. Existing API
> doesn't have this problem.
>
>
> If that is the only concern, would an additional convenience function
> solve (= sufficiently hide) the problem?
>
>   Instruction::setMergedDebugLoc(DebugLoc A, DebugLoc B) {
>     setDebugLoc(A);
>     mergeDebugLocWith(B);
>   }
>
> The primary benefit of having the API on Instruction is that we can make
> it impossible to accidentally create a locationless call instruction in a
> function with debuginfo (which would crash/assert the backend when creating
> inline scopes).
>

Either way we have a bug - either the profile will be buggy or the inlining
broken, honestly since the buggy profile is harder to diagnose (because it
has a location, but an incorrect one) I'd sort of favor an API that makes
those bugs easy/the default, rather than the subtlety of incorrect
locations.


>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D29833
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170223/a654b084/attachment.html>