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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
