aaron.ballman added a comment. In D59523#1440238 <https://reviews.llvm.org/D59523#1440238>, @jfb wrote:
> In D59523#1440232 <https://reviews.llvm.org/D59523#1440232>, @aaronpuchert > wrote: > > > > It's less about the regressions and more about the kind of regressions > > > we're testing against. > > > > But the test also verifies that no diagnostics are omitted (`// > > expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. > > Which is why I think it's a nice seed to build more systematic tests around > > it. I'd generally like to test this more systematically, but that isn't > > made easier if we're sprinkling the test cases over the code base. > > > No, I wrote the test purely to check that the crash is gone. These tests > *require* a diagnostic check, or `// expected-no-diagnostics`, so I put the > later. They only require `// expected-no-diagnostics` because you pass in `-verify` on the RUN line. That isn't needed to test the crashing regression, which may be part of what's causing confusion. > Agreed that ObjC needs to be tested more systematically, but this patch isn't > doing this. It's fixing a crash, and adding a test to make sure the crash is > gone. > >>> Basically, this file is here to prevent regressions. >> >> Isn't every test about exactly that? That there was a similar bug in the >> past is at best informative, but not necessarily relevant. And if a >> functional test crashes, isn't that just as bad? I don't understand why the >> historical reason for a test needs to have an influence on where the test is >> placed. It makes much more sense to me to group tests by the code they test. >> >> If there is a unit test for a class and you find a bug in it that makes it >> crash, would you write a complete new unit test? Or would you add a test >> case to the existing test? These files are our “poor man's unit test” for >> warnings. > > This is very concrete: this specific code used to cause a crash. This test > has exactly this purpose, nothing more. What I'm doing is extremely normal > for LLVM. Agreed, this is a normal practice for tests verifying that a crash no longer happens. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59523/new/ https://reviews.llvm.org/D59523 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
