[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert 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:
>
> > This is very concrete: this specific code used to cause a crash. This test has exactly this purpose, nothing more. What I'm doing is extremely normal for LLVM.
>
>
> Agreed, this is a normal practice for tests verifying that a crash no longer happens.


It certainly makes sense in many components, and I'm not questioning their judgement. But in the seven years prior to my involvement, no one thought it necessary to create a separate test for crashes in the thread safety analysis, and it's not because there haven't been any.

Warnings are in a different situations than the parser, optimizer passes or the backend, as they only operate read-only on the AST (or CFG). Looking at the commit log for `test/SemaCXX`, the majority of recent commits that fix crashes (and say so in the commit message) have been extending existing test cases instead of creating new ones. (Take for example rC337766 <https://reviews.llvm.org/rC337766>, rC338089 <https://reviews.llvm.org/rC338089>, rC342192 <https://reviews.llvm.org/rC342192>, rC352047 <https://reviews.llvm.org/rC352047>.) The thread safety analysis only looks at a single function's source-level CFG at a time. So we can reasonably consider functions as sufficiently isolated test cases and declarations as their setup.

That this doesn't work for a crash in the backend or parser is completely obvious to me. When working there, I would probably do the same and create a new file to reproduce my case in isolation. But what makes sense and is established practice in one component might not always make sense in another.

I'll leave the judgement to you, I'm just not convinced that this needs to be done.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523