[PATCH] D132111: [clang][Interp] Implement pointer (de)ref operations and DeclRefExprs

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:613-614
   case UO_LNot: // !x
+    if (!this->Visit(SubExpr))
+      return false;
     return this->emitInvBool(E);
----------------
I don't love that the `Visit()` calls are now repeated for most of the operations, but I don't have a better suggestion. I see that `Visit()` must not be called for ``UO_Deref` since `dereference()` will perform the visitation. I guess that case could be pulled out of the switch, but that isn't necessarily better. Consider this an unactionable comment.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:662
+      return this->emitGetPtrParam(It->second, E);
+  }
+
----------------
Perhaps add:
  else {
    assert(0 && "Unhandled declaration kind");
  }


================
Comment at: clang/test/AST/Interp/cxx20.cpp:2
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++20 -verify %s
+// RUN: %clang_cc1 -std=c++20 -verify %s -DREFERENCE
+
----------------
Is `-DREFERENCE` needed here? I don't see `REFERENCE` referenced in the test. From the other test, I gather that it is intended to annotate differences from the "reference" implementation.


================
Comment at: clang/test/AST/Interp/cxx20.cpp:11
+  return *p;
+}
+
----------------
Perhaps add:
  //static_assert(getMinux5() == -5, "");  TODO


================
Comment at: clang/test/AST/Interp/literals.cpp:43
+  return &m;
+}
+
----------------
Perhaps add:
  //static_assert(*getIntPointer() == 10, ""); TODO


================
Comment at: clang/test/AST/Interp/literals.cpp:47
+  return k;
+}
----------------
Perhaps add:
  //static_assert(gimme(5) == 5, ""); TODO


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

https://reviews.llvm.org/D132111