One existing problem is that we can't do much with the values since comparing with nans is broken ATM and I don't want to make this patch depend on the __builtin_bit_cast patch.
Details
Diff Detail
Event Timeline
clang/lib/AST/Interp/InterpBuiltin.cpp | ||
---|---|---|
88 | Please add test coverage for a case like __builtin_nan("derp") to show it's not a valid constant expression. | |
106 | LOL, thank you for this comment and wow. :-D | |
131–133 | We should have test coverage for these odd sizes just to ensure we're properly pushing/popping what we expect off the stack. | |
clang/test/AST/Interp/builtin-functions.cpp | ||
44 | Probably worth a comment here mentioning that the ref-error is a rejects-valid issue that the experimental compiler accepts correctly. |
clang/test/AST/Interp/builtin-functions.cpp | ||
---|---|---|
44 | I thought you said on Discord that the current interpreter is correct? I made the new one reject anything but string literals in https://reviews.llvm.org/D155545. |
clang/test/AST/Interp/builtin-functions.cpp | ||
---|---|---|
44 | Oops, crossing streams as I just mentioned this in another review of yours in the same area. I think I misunderstood the question on Discord when we were chatting. I was thinking the situation was more that Sema was allowing through something like __builtin_nan(12); and diagnosing it later during CodeGen. I think constexpr builtins should generally behave the same as regular constexpr functions the user could write. I'm really sorry if I made extra work for you with this confusion! |
One thing I'm wondering though is:
constexpr char A[] = {'1', '2', '3'}; constexpr double d = __builtin_nans(A);
what's expected here? The given char array is not null-terminated.
Should the interpreter ignore the null byte at the end if it's there? StringRef::getAsInteger() always returns an error if it encounters one, so the currentcode in InterpBuiltin.cpp ignores the last byte and will pass 12 along for the example above.
The builtin expects a null terminated string, so that should be an invalid constant expression due to reading off the end of the array.
Should the interpreter ignore the null byte at the end if it's there? StringRef::getAsInteger() always returns an error if it encounters one, so the currentcode in InterpBuiltin.cpp ignores the last byte and will pass 12 along for the example above.
Yeah, we should not do that. :-D In GCC, this builtin exists to support the nans() function from https://www.open-std.org/jtc1/sc22/wg14/www/docs/n965.htm (which was proposed but never adopted); Clang's support is for parity with GCC.
clang/test/AST/Interp/builtin-functions.cpp | ||
---|---|---|
53 | So answer both your questions: Yes, but the current interpreter has a isa<StringLiteral>() check and rejects everything that isn't a string literal. |
LG!
clang/test/AST/Interp/builtin-functions.cpp | ||
---|---|---|
53 | Ah! Slap some FIXME comments around the two cases to explain what's going on and that can be solved separately. Seem reasonable? |
clang/test/AST/Interp/builtin-functions.cpp | ||
---|---|---|
53 | Sure, I can do that. |
Please add test coverage for a case like __builtin_nan("derp") to show it's not a valid constant expression.