[clang] [clang][bytecode] Fix conditonal op scoping (PR #201777)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

<details>
<summary>Changes</summary>

The code and the comment are correct, but they also applied to the conditional operator condition, which they shouldn't.

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


2 Files Affected:

- (modified) clang/lib/AST/ByteCode/Compiler.cpp (+11-10) 
- (modified) clang/test/AST/ByteCode/cxx20.cpp (+17) 


``````````diff
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index 732a9e227430c..f5c7700b9cde7 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -2834,16 +2834,6 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
     return this->delegate(FalseExpr);
   }
 
-  // Force-init the scope, which creates a InitScope op. This is necessary so
-  // the scope is not only initialized in one arm of the conditional operator.
-  this->VarScope->forceInit();
-  // The TrueExpr and FalseExpr of a conditional operator do _not_ create a
-  // scope, which means the local variables created within them unconditionally
-  // always exist. However, we need to later differentiate which branch was
-  // taken and only destroy the varibles of the active branch. This is what the
-  // "enabled" flags on local variables are used for.
-  llvm::SaveAndRestore LAAA(this->VarScope->LocalsAlwaysEnabled,
-                            /*NewValue=*/false);
   bool IsBcpCall = false;
   if (const auto *CE = dyn_cast<CallExpr>(Condition->IgnoreParenCasts());
       CE && CE->getBuiltinCallee() == Builtin::BI__builtin_constant_p) {
@@ -2871,6 +2861,17 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
     return false;
   }
 
+  // Force-init the scope, which creates a InitScope op. This is necessary so
+  // the scope is not only initialized in one arm of the conditional operator.
+  this->VarScope->forceInit();
+  // The TrueExpr and FalseExpr of a conditional operator do _not_ create a
+  // scope, which means the local variables created within them unconditionally
+  // always exist. However, we need to later differentiate which branch was
+  // taken and only destroy the varibles of the active branch. This is what the
+  // "enabled" flags on local variables are used for.
+  llvm::SaveAndRestore LAAA(this->VarScope->LocalsAlwaysEnabled,
+                            /*NewValue=*/false);
+
   if (!this->jumpFalse(LabelFalse, E))
     return false;
   if (!this->delegate(TrueExpr))
diff --git a/clang/test/AST/ByteCode/cxx20.cpp b/clang/test/AST/ByteCode/cxx20.cpp
index d3e6265a9cbe7..a5a3a3346dad4 100644
--- a/clang/test/AST/ByteCode/cxx20.cpp
+++ b/clang/test/AST/ByteCode/cxx20.cpp
@@ -1262,6 +1262,23 @@ namespace ConditionalTemporaries {
   }
   static_assert(foo(false)== 13);
   static_assert(foo(true)== 12);
+
+  struct S {
+    int *p;
+    constexpr S() { p = new int(); }
+    constexpr ~S() { delete p; }
+  };
+
+  constexpr bool func(S &s1, S s2) {
+    return true;
+  }
+
+  constexpr bool test() {
+    S s1;
+    func(s1, {}) ? void() : void();
+    return true;
+  }
+  static_assert(test());
 }
 
 namespace PointerCmp {

``````````

</details>


https://github.com/llvm/llvm-project/pull/201777