aaronpuchert added inline comments.
================ Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s + ---------------- aaron.ballman wrote: > jfb wrote: > > aaron.ballman wrote: > > > jfb wrote: > > > > aaron.ballman wrote: > > > > > aaronpuchert wrote: > > > > > > jfb wrote: > > > > > > > aaronpuchert wrote: > > > > > > > > Test is fine for me, but I would like if you could integrate it > > > > > > > > with the existing > > > > > > > > test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread > > > > > > > > safety analysis requires a bit of setup, that's why we tend to > > > > > > > > keep the tests in one file. I'll admit that the C++ tests have > > > > > > > > grown quite large, but for ObjC++ it's still very manageable. > > > > > > > Sure thing! I created a header that's shared and simplified this > > > > > > > repro a bit. I validated that the shared code was crashing before > > > > > > > and this fixes the crash. > > > > > > I would still like all ObjCXX tests for -Wthread-safety in one > > > > > > file, because that's what we have for the other languages as well. > > > > > > (more or less) > > > > > > > > > > > > It's somewhat easier to see which aspects are tested then and which > > > > > > are not. Right now we only have tests for reported crashes, but > > > > > > maybe someday someone finds the time to test a bit more > > > > > > exhaustively. > > > > > > > > > > > > But perhaps @aaron.ballman sees this another way, in that case > > > > > > follow his opinion. > > > > > On one hand, the all-in-one test files get to be unwieldy size, but > > > > > on the other hand, it's nice having everything integrated into a > > > > > single file for testing purposes. I'd say go with the all-in-one > > > > > approach because it's consistent with how we otherwise test thread > > > > > safety and there are some (albeit, minor) benefits. > > > > I'm not a fan of huge tests, and it's not consistent with other parts > > > > of LLVM. Specifically, this is testing for a regression, and should > > > > standalone to future edits don't refactor the test away. The existing > > > > test checks for functionality, and refactoring it is fine. > > > I'm not going to insist, but the two people who do the most active work > > > in this area in recent history just told you what they prefer. ;-) We add > > > regression tests to the existing thread safety test files and it's worked > > > out just fine. > > 4% of files under clang/test (and llvm/test) start with "PR". I appreciate > > your being gentle... and I'll gently nudge folks working on thread safety > > to follow the established process clang and LLVM follow ;-) > > Maybe I should file a bug to follow said process myself? > > > > Basically, this file is here to prevent regressions. It really shouldn't > > change, ever, and I'd like to make this explicit. > Thanks to an IRC discussion, I now understand the point @jfb is making about > this being a crashing regression test. You're right, we don't usually > incorporate those into larger functionality tests. It's less about the > regressions and more about the kind of regressions we're testing against. > > I'm fine with the approach in this patch now. The other test is not really a functional test, I added it in rC342600 as part of a fix for another bug that was reported as [PR38896](https://bugs.llvm.org/show_bug.cgi?id=38896). But the underlying cause for both bugs is not an oversight, it's that ObjC[++] is just not supported, and someone was too lazy to completely turn the analysis off or disallow it. Now people try it out and unsurprisingly it doesn't work. Your test is not some weird special case, so it could become part of a more systematic functional test. In Clang it's quite usual to have one test per warning flag, and the thread safety analysis is just one flag. I guess the reason is that it makes refactoring easier: you see all the special cases in one file. That's quite nice! Have a look at some of these tests, they basically allow you to write the warning yourself. If all these special cases were spread around the code base, that would certainly make it harder to understand what a warning does. The test for C++ also contains tests for at least two bugs: PR34800, PR38640. (I just grepped this, there is probably more. You can try `grep PR test/SemaCXX/warn-*`.) If you fear someone is going to change the test, rename your class from `MyInterface` to `PRXXXXX` or something else which indicates a crash. Also indicate the issue it detects: that ObjC methods weren't handled in `SExprBuilder::translateDeclRefExpr`. Or that we test that they are handled now. 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
