[PATCH] D51867: [Diagnostics] Add error handling to FormatDiagnostic()

[Jan Korous via Phabricator via cfe-commits]

jkorous planned changes to this revision.
jkorous added a comment.

Hi Volodymyr, 
Thanks for the feedback - interesting points!

I see your point regarding NFC - I am going to drop it as you are right.

Two things about sanitizers come to mind:

1. You'd need to guarantee that you have all possible code paths (or at least those you are able to cover with asserts) covered in tests to be able to replace asserts with sanitizers. (I think that even if that would be feasible asserts would prove to be way simpler.)
2. I prefer explicit assert right in the place where an assumption is made to test somewhere else as it make understanding the code much easier.

> Those change loop iteration from "iterator != end" to "iterator < end". As it is functionality change, I'd like to have tests to cover that.

Technically you are right but I assume (ok, busted) that without any bug in the iteration this is NFC. I will try to look if I can find some simple input that would break current implementation.

> Also I've fixed a few bugs with going past the end of buffer and bugs were actually inside the loop, not with buffer range check. It is tempting to play safe but it has a risk of hiding real bugs.

But that almost sounds as that we should write fragile code so bugs from other parts of codebase show up... Anyway, I will think about this a bit more, it's an interesting point.



================
Comment at: lib/Basic/Diagnostic.cpp:804
+
+  while (DiagStr < DiagEnd) {
     if (DiagStr[0] != '%') {
----------------
vsapsai wrote:
> For example, I wouldn't call this one NFC.
You are right, I am gonna drop the NFC.


Repository:
  rC Clang

https://reviews.llvm.org/D51867