[clang][Interp] Record initialization via conditional operator

[tbaeder]

Summary

Factor this out into a visitConditional() that calls the given visitor function.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

934

This meant to be here?

935

Is this condition supposed to be just a 'visit', or should this be using the functor?

clang/lib/AST/Interp/ByteCodeExprGen.h

183

I'd probably rather make this a template taking a functor, rather than bringing in the atrocities that come with std::function.

clang/test/AST/Interp/records.cpp

339

Did this not work before? I don't see what part of teh code changes makes this work?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

934

Nope! :(

935

The visit is correct here, the functor is just for the true/false expr.

clang/lib/AST/Interp/ByteCodeExprGen.h

183

Alright, will look into that

clang/test/AST/Interp/records.cpp

339

We call visitInitializer also when the return value is constructed in the calling function, so the return statement in getS() exercises the code added.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

183

Done. Not sure if this is exactly how you meant it to look. Feels a bit inelegant.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.h

183

This actually probably needs to be a non-member now, since its only used in the .cpp.

while I DO dislike that it was using std::function (probably because of captures in the lambdas?), I now suspect this is worse if we can't have it in the header, right?

[tbaeder]

Ping

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.h

182–183

The template definition isn't available within the header file, so this is fragile (impossible to instantiate from anything but ByteCodeExprGen.cpp).

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

182–183

What's the alternative? If I make it a static function in ByteCodeExprGen.cpp, I'd have to make a lot of members of ByteCodeEmitter etc. public which is not a very clean solution. Moving the definition into the header file isn't very nice either, all the others are in the source file.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.h

182–183

What about making it a static template in ByteCodeExprGen.cpp, but make it a 'friend' here? I think that would work, wouldn't it? Something like: https://godbolt.org/z/ofYbGvfYa

You unfortunately cannot make it static, but as it is a template, at least it is inline. WDYT?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

182–183

I tried that but I keep running into linker problems:

mold: error: undefined symbol: lib/libclangAST.a(ByteCodeExprGen.cpp.o): bool clang::interp::visitConditional<clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>::VisitAbstractConditionalOperator(clang::AbstractConditionalOperator const*)::{lambda(clang::Expr const*)#1}>(clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>*, clang::AbstractConditionalOperator const*, clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>::VisitAbstractConditionalOperator(clang::AbstractConditionalOperator const*)::{lambda(clang::Expr const*)#1})

(and another one for EvalEmitter). But I can't investigate more before Wednesday.

[tbaeder]

Screw it, just use a llvm::function_ref.

[aaron.ballman]

I'm fine with this approach.

clang/lib/AST/Interp/ByteCodeExprGen.h

if (!this->emitThis(I))
       return false;
 
     if (!visitInitializer(I))
       return false;
 
     return this->emitPopPtr(I);
   }
 
+  template <typename VisitFn>
+  bool visitConditional(const AbstractConditionalOperator *E, VisitFn V);
+
   /// Creates a local primitive value.
   unsigned allocateLocalPrimitive(DeclTy &&Decl, PrimType Ty, bool IsMutable,
                                   bool IsExtended = false);
 
   /// Allocates a space storing a local given its type.
   std::optional<unsigned> allocateLocal(DeclTy &&Decl, bool IsExtended = false);
 
 private:

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
   return this->visit(E->getSourceExpr());
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitAbstractConditionalOperator(
     const AbstractConditionalOperator *E) {
-  const Expr *Condition = E->getCond();
-  const Expr *TrueExpr = E->getTrueExpr();
-  const Expr *FalseExpr = E->getFalseExpr();
-
-  LabelTy LabelEnd = this->getLabel();   // Label after the operator.
-  LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
-
-  if (!this->visit(Condition))
-    return false;
-  if (!this->jumpFalse(LabelFalse))
-    return false;
-
-  if (!this->visit(TrueExpr))
-    return false;
-  if (!this->jump(LabelEnd))
-    return false;
-
-  this->emitLabel(LabelFalse);
-
-  if (!this->visit(FalseExpr))
-    return false;
-
-  this->fallthrough(LabelEnd);
-  this->emitLabel(LabelEnd);
-
-  return true;
+  return this->visitConditional(
+      E, [this](const Expr *E) { return this->visit(E); });
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitStringLiteral(const StringLiteral *E) {
   unsigned StringIndex = P.createGlobalString(E);
   return this->emitGetPtrGlobal(StringIndex, E);
 }
 

 bool ByteCodeExprGen<Emitter>::visitBool(const Expr *E) {
   if (std::optional<PrimType> T = classify(E->getType())) {
     return visit(E);
   } else {
     return this->bail(E);
   }
 }
 
+/// Visit a conditional operator, i.e. `A ? B : C`.
+/// \V determines what function to call for the B and C expressions.
+template <class Emitter>
+template <typename VisitFn>
+bool ByteCodeExprGen<Emitter>::visitConditional(
+    const AbstractConditionalOperator *E, VisitFn V) {
+
+  const Expr *Condition = E->getCond();
+  const Expr *TrueExpr = E->getTrueExpr();
+  const Expr *FalseExpr = E->getFalseExpr();
+
+  LabelTy LabelEnd = this->getLabel();   // Label after the operator.
+  LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
+
+  if (!this->visit(Condition))
+    return false;
+  if (!this->jumpFalse(LabelFalse))
+    return false;
+
+  if (!V(TrueExpr))
+    return false;
+  if (!this->jump(LabelEnd))
+    return false;
+
+  this->emitLabel(LabelFalse);
+
+  if (!V(FalseExpr))
+    return false;
+
+  this->fallthrough(LabelEnd);
+  this->emitLabel(LabelEnd);
+
+  return true;
+}
+
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitZeroInitializer(PrimType T, const Expr *E) {
   switch (T) {
   case PT_Bool:
     return this->emitZeroBool(E);
   case PT_Sint8:
     return this->emitZeroSint8(E);
   case PT_Uint8:
if (const auto CtorExpr = dyn_cast<CXXConstructExpr>(Initializer)) {
 
     return this->VisitCallExpr(CE);
   } else if (const auto *DIE = dyn_cast<CXXDefaultInitExpr>(Initializer)) {
     return this->visitInitializer(DIE->getExpr());
   } else if (const auto *CE = dyn_cast<CastExpr>(Initializer)) {
     return this->visitInitializer(CE->getSubExpr());
   } else if (const auto *CE = dyn_cast<CXXBindTemporaryExpr>(Initializer)) {
     return this->visitInitializer(CE->getSubExpr());
+  } else if (const auto *ACO =
+                 dyn_cast<AbstractConditionalOperator>(Initializer)) {
+    return this->visitConditional(
+        ACO, [this](const Expr *E) { return this->visitRecordInitializer(E); });
   }
 
   return false;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitInitializer(const Expr *Initializer) {
   QualType InitializerType = Initializer->getType();

clang/test/AST/Interp/records.cpp

constexpr YetAnotherDerived Oops(0); // ref-error {{must be initialized by a constant expression}} \
                                        // FIXME: Missing reason for rejection.
 };
 
 namespace EmptyCtor {
   struct piecewise_construct_t { explicit piecewise_construct_t() = default; };
   constexpr piecewise_construct_t piecewise_construct =
     piecewise_construct_t();
 };
+
+namespace ConditionalInit {
+  struct S { int a; };
+
+  constexpr S getS(bool b) {
+    return b ? S{12} : S{13};
+  }
+
+  static_assert(getS(true).a == 12, "");
+  static_assert(getS(false).a == 13, "");
+};