jfb added a comment. 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. 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. 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
