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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> It's less about the regressions and more about the kind of regressions we're testing against.

But the test also verifies that no diagnostics are omitted (`// expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. Which is why I think it's a nice seed to build more systematic tests around it. I'd generally like to test this more systematically, but that isn't made easier if we're sprinkling the test cases over the code base.

> Basically, this file is here to prevent regressions.

Isn't every test about exactly that? That there was a similar bug in the past is at best informative, but not necessarily relevant. And if a functional test crashes, isn't that just as bad? I don't understand why the historical reason for a test needs to have an influence on where the test is placed. It makes much more sense to me to group tests by the code they test.

If there is a unit test for a class and you find a bug in it that makes it crash, would you write a complete new unit test? Or would you add a test case to the existing test? These files are our “poor man's unit test” for warnings.



================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282
+    if (Ctx && Ctx->FunArgs) {
+      const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+      if (isa<FunctionDecl>(D)
----------------
jfb wrote:
> aaron.ballman wrote:
> > Same.
> It's a `Decl` (that's canonical), but same.
Depends on the type of `Ctx->AttrDecl`, some derived types of `Decl` have a stricter return type for `getCanonicalDecl()`. So I guess it boils down to what one thinks is [obvious](https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable). We're erring on the “be explicit” end of the scale here.


================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+        assert(I < Ctx->NumArgs);
+        return translate(Ctx->FunArgs[I], Ctx->Prev);
+      }
----------------
jfb wrote:
> aaronpuchert wrote:
> > Does your test run into this with an `ObjCMethodDecl`? I see how we run into the assignment to `VD` down below, but I don't understand how we could get here without annotating a method.
> The problem is in the cast above:
> ```
> (lldb) 
> frame #5: clang::threadSafety::SExprBuilder::translateDeclRefExpr(this, DRE, Ctx) at ThreadSafetyCommon.cpp:280:9
>    277 	  // Function parameters require substitution and/or renaming.
>    278 	  if (const auto *PV = dyn_cast_or_null<ParmVarDecl>(VD)) {
>    279 	    const auto *FD =
> -> 280 	        cast<FunctionDecl>(PV->getDeclContext())->getCanonicalDecl();
>    281 	    unsigned I = PV->getFunctionScopeIndex();
>    282 	
>    283 	    if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
> ```
Ok. I just thought it would be interesting to see if can we run into the `if`, but I can try that myself.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523