[PATCH] D155356: [clang][Interp] Implement __builtin_nan family of functions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/InterpBuiltin.cpp:88
+  else if (StringRef(Str).getAsInteger(0, Fill))
+    return false;
+
----------------
Please add test coverage for a case like `__builtin_nan("derp")` to show it's not a valid constant expression.


================
Comment at: clang/lib/AST/Interp/InterpBuiltin.cpp:106
+    // 2008 revisions, MIPS interpreted sNaN-2008 as qNan and qNaN-2008 as
+    // sNaN. This is now known as "legacy NaN" encoding.
+    if (Signaling)
----------------
LOL, thank you for this comment and wow. :-D


================
Comment at: clang/lib/AST/Interp/InterpBuiltin.cpp:135-137
+  case Builtin::BI__builtin_nanl:
+  case Builtin::BI__builtin_nanf16:
+  case Builtin::BI__builtin_nanf128:
----------------
We should have test coverage for these odd sizes just to ensure we're properly pushing/popping what we expect off the stack.


================
Comment at: clang/test/AST/Interp/builtin-functions.cpp:44
+
+  constexpr float Nan2 = __builtin_nans([](){return "0xAE98";}()); // ref-error {{must be initialized by a constant expression}}
+}
----------------
Probably worth a comment here mentioning that the ref-error is a rejects-valid issue that the experimental compiler accepts correctly.


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

https://reviews.llvm.org/D155356