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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
