[PATCH] D132286: [clang][Interp] Implement function calls

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:603
+  if (const auto *FuncDecl = dyn_cast_or_null<FunctionDecl>(Callee)) {
+    Function *Func = P.getFunction(FuncDecl);
+
----------------



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:614
+
+    if (Optional<PrimType> T = classify(E->getType())) {
+      // Put arguments on the stack.
----------------
Should this be using `CallExpr::getCallReturnType()` instead? (with test cases demonstrating the difference between `getType()` and this call)


================
Comment at: clang/lib/AST/Interp/Interp.cpp:409-411
 }
+
 bool Interpret(InterpState &S, APValue &Result) {
----------------
Spurious whitespace change?


================
Comment at: clang/lib/AST/Interp/Interp.cpp:60
+  S.Current =
+      new InterpFrame(S, const_cast<Function *>(Func), S.Current, PC, {});
+
----------------
tbaeder wrote:
> This patch adds two `const_cast`s. They aren't strictly necessary, since the `InterpFrame` doesn't need to modify the function. I have a local NFC patch to add `const` to a few places to make this work.
Thanks! FWIW, because this is an entirely new interface for the compiler, I'm hoping we can force some better const correctness in this part of the code base.


================
Comment at: clang/lib/AST/Interp/InterpFrame.cpp:174
 
+  assert(Args);
   // Copy the initial value.
----------------
Can you add a message to this assert so folks know what broke when you got here. Also, what about functions with no arguments?


================
Comment at: clang/lib/AST/Interp/PrimType.h:85
   case Name: { using T = PrimConv<Name>::T; B; break; }
+
 #define TYPE_SWITCH(Expr, B)                                                   \
----------------
Spurious whitespace change?


================
Comment at: clang/test/AST/Interp/functions.cpp:1-2
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++14 -verify %s
+// RUN: %clang_cc1 -std=c++14 -verify=ref %s
+
----------------
I don't think we need to limit the testing to just c++14


================
Comment at: clang/test/AST/Interp/functions.cpp:8-9
+template<typename T> constexpr T identity(T t) { return t; }
+static_assert(identity(true), "");
+static_assert(identity(true), "");
+static_assert(!identity(false), "");
----------------
Why testing twice?


================
Comment at: clang/test/AST/Interp/functions.cpp:48
+}
+static_assert(sub(10, 8, 2) == 0, "");
----------------
It's a bit fuzzy where to draw the line on test coverage as this example is partially about function calls and partially about reference tracking. But I think we need more complex tests than what we've got so far.
```
constexpr void func(int &ref) {
  ref = 12;
}
constexpr int do_it() {
  int mutate_me = 0;
  func(mutate_me);
  return mutate_me;
}
static_assert(do_it() == 12, "");

constexpr int &get_ref(int &i) {
  return i;
}
constexpr int do_it_again() {
  int i = 0;
  get_ref(i) = 12;
  return i;
}
static_assert(do_it_again() == 12, "");
```

(Also, while working on this interpreter, please keep in mind that C is also looking to add support for constant expressions; C2x will have `constexpr` object definitions and we expect to widen that functionality for C2y. But this means you should keep in mind the crazy things that C can do too, like:
```
int oh_god_this_hurts(struct { int a; } s) {
  return s.a;
}
```
which could someday get a `constexpr` slapped in front of it.)

Speaking of C, remember that we also have C extensions, like using VM types, that should have test coverage:
```
constexpr int func(int n, int array[static n], int m) {
  return array[m];
}

constexpr int do_it() {
  int array[] = { 0, 1, 2, 3, 4 };
  return func(5, array, 1);
}

static_assert(do_it() == 1, "");
```


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

https://reviews.llvm.org/D132286