[PATCH] D59523: Thread Safety: also look at ObjC methods

JF Bastien via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 15:15:37 PDT 2019


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





More information about the cfe-commits mailing list