NoQ requested changes to this revision. NoQ added a comment. This revision now requires changes to proceed.
In D81718#2095965 <https://reviews.llvm.org/D81718#2095965>, @baloghadamsoftware wrote: > Your test case unfortunately does not test what you want, because raw > pointers are not supported yet It tests exactly what i want: the correctness of your code in //this very patch// that was written to handle //this very case// for which you never even bothered figuring out the correct solution but you already wrote //a large amount of code// (including some code in this patch) //with zero test coverage// to demonstrate correctness of your solution. As of now you wrote your new two functions to return some value in every case. However, only some of these cases are covered with tests; in other cases the value was basically chosen randomly. The way you added unittests in order to make sure this completely random value stays random rather than correct doesn't count as testing. In D81718#2096020 <https://reviews.llvm.org/D81718#2096020>, @baloghadamsoftware wrote: > What is the correct solution then? To put branches all over the code of the > checkers? Surely not. That's literally how test-driven development works. First you write tests, then you write any code that covers them, and //only then// you figure out how to reuse code. This is because correctness is completely orthogonal to prettiness; arguing that my solution is incorrect "because it's ugly" is ridiculous. This is why test-driven development recommends starting with a correct-but-ugly solution, not with an incorrect-but-pretty solution: first you figure out how to solve the problem at all, then refactor. I will not discuss this further unless all the dead code that you're refactoring is either covered with LIT tests or removed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81718/new/ https://reviews.llvm.org/D81718 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
