[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 10:24:12 PDT 2019


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





More information about the cfe-commits mailing list