[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:55:58 PDT 2019


jfb added a comment.

In D59523#1440263 <https://reviews.llvm.org/D59523#1440263>, @aaron.ballman wrote:

> 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.


Thanks for pointing this out! I've removed both.


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