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

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

tbaeder marked 4 inline comments as done.
tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:614
+
+    if (Optional<PrimType> T = classify(E->getType())) {
+      // Put arguments on the stack.
----------------
aaron.ballman wrote:
> Should this be using `CallExpr::getCallReturnType()` instead? (with test cases demonstrating the difference between `getType()` and this call)
Pretty sure yes, I just didn't know about it. Can the difference only be demonstrated by using references?


================
Comment at: clang/lib/AST/Interp/Interp.cpp:409-411
 }
+
 bool Interpret(InterpState &S, APValue &Result) {
----------------
aaron.ballman wrote:
> Spurious whitespace change?
Yes, but again on purpose.


================
Comment at: clang/lib/AST/Interp/InterpFrame.cpp:174
 
+  assert(Args);
   // Copy the initial value.
----------------
aaron.ballman wrote:
> Can you add a message to this assert so folks know what broke when you got here. Also, what about functions with no arguments?
I'll move it into `stackRef()`, where the problem happened and where it's clear that `Args` should really be set at this point.


================
Comment at: clang/lib/AST/Interp/PrimType.h:85
   case Name: { using T = PrimConv<Name>::T; B; break; }
+
 #define TYPE_SWITCH(Expr, B)                                                   \
----------------
aaron.ballman wrote:
> Spurious whitespace change?
It's on purpose, I was just trying to improve this part. But I can remove it.


================
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
+
----------------
aaron.ballman wrote:
> I don't think we need to limit the testing to just c++14
Isn't it now just testing whatever the default is when the test is run?


================
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), "");
----------------
aaron.ballman wrote:
> Why testing twice?
The first call will compile the function in `ByteCodeExprGen::VisitCallExpr()` and execute it, the second one checks that visiting the same call again works. I can't test that the result is actually cached and that the compilation only happens once.


================
Comment at: clang/test/AST/Interp/functions.cpp:48
+}
+static_assert(sub(10, 8, 2) == 0, "");
----------------
aaron.ballman wrote:
> 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, "");
> ```
TBH I think references can be implemented and tested in a separate, later patch. Since I haven't tested them at all until now.


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

https://reviews.llvm.org/D132286