[PATCH] D136423: [clang][Interp] Implement inc/dec postfix and prefix operators

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1237-1240
+    if (DiscardResult)
+      return this->emitIncPop(*T, E);
+
+    return this->emitInc(*T, E);
----------------
Style question: should we prefer doing something like: `return DiscardResult ? this->emitIncPop(*T, E) : this->emitInc(*T, E);` to discourage accidentally adding code between the two increment operations? (And applying this style more generally for discarded results?)

Alternatively, do we want to add a helper function to `ByteCodeExprGen` called `doEmitInc()` that hides the discarded result behavior so the `Visit*` functions don't have to keep doing that dance?


================
Comment at: clang/lib/AST/Interp/Interp.h:331
 
+template <typename T, bool IsInc, bool PushVal>
+bool IncDecHelper(InterpState &S, CodePtr OpPC, const Pointer &Ptr) {
----------------
Do we want to use enums for `IsInc` and `PushVal` so that call sites use `IncDeclHelper<SomeType, Increment::Yes, Push::Yes>` for clarity?


================
Comment at: clang/lib/AST/Interp/Interp.h:370-373
+  } else {
+    S.CCEDiag(E, diag::note_constexpr_overflow) << APResult << Type;
+    return S.noteUndefinedBehavior();
+  }
----------------



================
Comment at: clang/test/AST/Interp/literals.cpp:376
+  static_assert(zero() == 0, "");
+
+  constexpr int three() {
----------------
I'd like some tests that the computed values are correct.
```
constexpr int zero() {
  int a = 0;
  return a++;
}
static_assert(zero() == 0);

constexpr int one() {
  int a = 0;
  return ++a;
}
static_assert(one() == 1);

constexpr int zero_again() {
  int a = 0;
  return a--;
}
static_assert(zero_again() == 0);

constexpr int neg_one() {
  int a = 0;
  return --a;
}
static_assert(neg_one() == -1);
```


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

https://reviews.llvm.org/D136423