[clang] Add a lifetime_capture_by testcase for temporary capturing object.

[hokein]

Add a test case to indicate this is an expected behavior.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Fixes #117728

---

Full diff: https://github.com/llvm/llvm-project/pull/117733.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+7-3)
• (modified) clang/test/Sema/warn-lifetime-analysis-capture-by.cpp (+11)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index a49605e4867651..54d8bbdfc0f4fd 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -3248,9 +3248,13 @@ void Sema::checkLifetimeCaptureBy(FunctionDecl *FD, bool IsMemberFunction,
       checkCaptureByLifetime(*this, CE, Captured);
     }
   };
-  for (unsigned I = 0; I < FD->getNumParams(); ++I)
-    HandleCaptureByAttr(FD->getParamDecl(I)->getAttr<LifetimeCaptureByAttr>(),
-                        I + IsMemberFunction);
+  // Suppress the warning if the capturing object is also a temporary to reduce
+  // noise, e.g `vector<string_view>().push_back(std::string());`.
+  if (!isa_and_present<MaterializeTemporaryExpr>(ThisArg)) {
+    for (unsigned I = 0; I < FD->getNumParams(); ++I)
+      HandleCaptureByAttr(FD->getParamDecl(I)->getAttr<LifetimeCaptureByAttr>(),
+                          I + IsMemberFunction);
+  }
   // Check when the implicit object param is captured.
   if (IsMemberFunction) {
     TypeSourceInfo *TSI = FD->getTypeSourceInfo();
diff --git a/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp b/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
index 4d562bac1e305b..61d13c9e585e44 100644
--- a/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
@@ -143,6 +143,17 @@ void use() {
 }
 } // namespace this_is_captured
 
+namespace ignore_temporary_class_object {
+struct S {
+  void add(const int& x [[clang::lifetime_capture_by(this)]]);
+};
+
+void test() {
+  S().add(1);
+  S{}.add(1);
+}
+} // namespace ignore_temporary_class_object
+
 // ****************************************************************************
 // Capture by Global and Unknown.
 // ****************************************************************************

[usx95]

I considered this during implementation. This is a little controversial.

It is possible that the temporary capturing object uses the captured entity in its destructor. In principle, we can always detect the order of destructions of the temporaries and choose to suppress cases when captured temporary outlives capturing temporary. But it would be a very confusing behaviour for the user, eg: switching order of params suppresses the warning.

I would be in favour of not doing this and always diagnosing this for simplicity until we see a strong need or practical false positives. WDYT ?

[hokein]

It is possible that the temporary capturing object uses the captured entity in its destructor.

This is a good point. We could potentially limit the suppression to cases where the destructor is trivial, but that might not be worth the added complexity.

Agree that this is probably not a critical issue -- I just noticed it while reading the code. I’m fine with leaving it as is. I have repurposed this PR to add a test case for it instead.

[usx95]

We could potentially limit the suppression to cases where the destructor is trivial, but that might not be worth the added complexity.
I agree that might not be worth the complexity. Let's revisit if we have practical false positives.

Thanks for the tests. LGTM.