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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

It looks like precommit CI found some relevant failures.



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:614
+
+    if (Optional<PrimType> T = classify(E->getType())) {
+      // Put arguments on the stack.
----------------
tbaeder wrote:
> 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?
I believe it's just references, but would have to go trace the logic through the compiler to be sure.


================
Comment at: clang/lib/AST/Interp/PrimType.h:85
   case Name: { using T = PrimConv<Name>::T; B; break; }
+
 #define TYPE_SWITCH(Expr, B)                                                   \
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Spurious whitespace change?
> It's on purpose, I was just trying to improve this part. But I can remove it.
It's the only change to the file, so it should be removed (you can make an NFC commit to fix all the whitespace issues though!)


================
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
+
----------------
tbaeder wrote:
> 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?
It is, but that's our preference for tests when possible because we don't want to lose testing in newer language modes by pegging the test to an older language mode (note, we're currently exploring changes to this so you can specify a range of standards: https://reviews.llvm.org/D131464).


================
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), "");
----------------
tbaeder wrote:
> 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.
Ah, that's good to know! That might be worth leaving a commend in the file so nobody thinks it's redundant.


================
Comment at: clang/test/AST/Interp/functions.cpp:48
+}
+static_assert(sub(10, 8, 2) == 0, "");
----------------
tbaeder wrote:
> 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.
That's fine by me!


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

https://reviews.llvm.org/D132286