jfb marked an inline comment as done. jfb 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: > 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. 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
