[PATCH] D95860: [clang][Frontend] Fix a crash in DiagnosticRenderer.

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

In D95860#2539453 <https://reviews.llvm.org/D95860#2539453>, @balazske wrote:

> Probably it is not worth to find a better solution. In the case of CloneChecker the range starts and ends on different line that is not possible to print on a single line in the output.
> This code could work:
> [code...]
> With this solution the following result is printed:
>
>   llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:9:3: warning: Duplicate code detected [alpha.clone.CloneChecker]
>     if (i == 10) // expected-warning{{Duplicate code detected}}
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:11:3: note: Similar code here
>     if (i == 10) // expected-note{{Similar code here}}
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   1 warning generated.
>
> This is not better than the result with the existing code:
>
>   llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:9:3: warning: Duplicate code detected [alpha.clone.CloneChecker]
>     if (i == 10) // expected-warning{{Duplicate code detected}}
>     ^
>   llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:11:3: note: Similar code here
>     if (i == 10) // expected-note{{Similar code here}}
>     ^
>   1 warning generated.

I think you are right. There is no gain in complicating this. I don't think there is anything that we can do about this.
Your original patch, simply ignoring the `end` if that is invalid or from a different file is fine by me.

Thank you for going the extra mile!

In D95860#2538845 <https://reviews.llvm.org/D95860#2538845>, @balazske wrote:

> I realized that the added test cases show different problems: One is when one of `Begin` or `End` is invalid, other if the FileID's are different. Probably we can find a better solution for the second problem. So the patch needs to be splitted and the case with the **crash-diagnostic-renderer.cpp** goes into the first part.

I don't mind committing this in one go.
However, whichever you choose, please update the revision summary according to the commit message.
You should definitely describe both of these issues you found.

Thanks for your effort.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95860