[PATCH] D137487: [clang][Interp] Start implementing builtin functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1374
   const Decl *Callee = E->getCalleeDecl();
-  if (const auto *FuncDecl = dyn_cast_or_null<FunctionDecl>(Callee)) {
+  if (const auto *FuncDecl = dyn_cast_if_present<FunctionDecl>(Callee)) {
     const Function *Func = getFunction(FuncDecl);
----------------
erichkeane wrote:
> This is an unrelated change, perhaps could be in an NFC commit.
Right, sure


================
Comment at: clang/lib/AST/Interp/Interp.h:1354
 
-  APValue CallResult;
-  // Note that we cannot assert(CallResult.hasValue()) here since
-  // Ret() above only sets the APValue if the curent frame doesn't
-  // have a caller set.
-  if (Interpret(S, CallResult)) {
-    NewFrame.release(); // Frame was delete'd already.
-    assert(S.Current == FrameBefore);
-
-    // For constructors, check that all fields have been initialized.
-    if (Func->isConstructor()) {
-      if (!CheckCtorCall(S, PC, ThisPtr))
-        return false;
+  // Evaluate builtin functions directly.
+  if (unsigned BID = Func->getBuiltinID()) {
----------------
erichkeane wrote:
> `directly` reads oddly here?  Do you mean the 'immediately' definition of this?
Yes, that was the purpose. I can reword it.


================
Comment at: clang/lib/AST/Interp/Interp.h:1355
+  // Evaluate builtin functions directly.
+  if (unsigned BID = Func->getBuiltinID()) {
+    if (InterpretBuiltin(S, PC, BID)) {
----------------
erichkeane wrote:
> Rather than the 'if'/'else' pair, do we just want to immediately return on the failed builtin?  
Sure, that's possible. I guess this ties into the question of whether to use a different opcode for builtin functions; but so far I have found builtin calls to be too similar to regular ones, esp. since they don't have a special AST node.


================
Comment at: clang/lib/AST/Interp/Interp.h:1357
+    if (InterpretBuiltin(S, PC, BID)) {
+      NewFrame.release();
+      return true;
----------------
erichkeane wrote:
> What is going on here?  Why is this not a leak?  
The current frame is `delete`d when successfully returning the value of the function, this happens in the `Ret` (and `RetVoid`) function in `Interp.cpp`.

No opinion on whether that's the best way to do it though.


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

https://reviews.llvm.org/D137487