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