[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D129755#3777038 <https://reviews.llvm.org/D129755#3777038>, @aaronpuchert wrote:

> I was under the impression that we've already switched to C++17, but the Windows pre-submit build failed with:
>
>   C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2107): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
>   C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2120): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
>   C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2418): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
>
> Perhaps I should move the init statements out again?

No, I've filed https://github.com/google/llvm-premerge-checks/issues/416 to find out what's going on with the Windows precommit CI bot.

The changes LGTM, though it'd still be great if @delesley had some time to put a second set of eyes on the changes. If we don't hear from him by Tue, I think we should go ahead and land this. Please be sure to add a release note for the changes!



================
Comment at: clang/lib/Analysis/ThreadSafetyCommon.cpp:339-342
+    if (const Expr *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
+      return translate(SelfArg, Ctx->Prev);
+    else
+      return cast<til::SExpr *>(Ctx->SelfArg);
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > 
> No doubt referring to https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return. I was trying to emphasize the dual nature of `llvm::PointerUnion<const Expr *, til::SExpr *>`, but I can certainly also drop the `else`.
I don't have a strong opinion.


================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4230-4236
+void testDeferredTemporary() {
+  SelfLockDeferred(); // expected-warning {{releasing mutex '<temporary>.mu_' that was not held}}
+}
+
+void testDeferredTemporary2() {
+  SelfLockDeferred2(); // expected-warning {{releasing mutex '<temporary>' that was not held}}
+}
----------------
aaronpuchert wrote:
> Here we're printing `<temporary>`, because we don't have a name for this object.
Ah thank you, this example makes a lot of sense. I think printing `<temporary>` is quite reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755