[PATCH] D134744: [clang][Interp] Implement rem opcode

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/AST/Interp/Integral.h:213
 
+  static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) {
+    *R = Integral(A.V % B.V);
----------------
Its a touch weird we return 'bool' in all these places, rather than an `Optional<Integral>`, right?  Perhaps a refactor idea for a future patch.


================
Comment at: clang/lib/AST/Interp/Interp.h:171
+  T Result;
+  T::rem(LHS, RHS, Bits, &Result);
+  S.Stk.push<T>(Result);
----------------
I get that we 'know' that rem doesn't have an error case, but it seems like a mistake to not check the return value... Perhaps an even better reason to use optional.


================
Comment at: clang/lib/AST/Interp/Opcodes.td:57
 
-def AluTypeClass : TypeClass {
+def NumberTypeClass : TypeClass {
   let Types = [Sint8, Uint8, Sint16, Uint16, Sint32,
----------------
Perhaps `IntegralTypeClass`, unless we expect floats to be here too?


================
Comment at: clang/lib/AST/Interp/Opcodes.td:62
+
+def AluTypeClass : TypeClass {
+  let Types = !listconcat(NumberTypeClass.Types, [Bool]);
----------------
What does "Alu" mean here?


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

https://reviews.llvm.org/D134744