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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
