tahonermann added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1098-1099
+
+ unsigned N = SL->getLength();
+ for (size_t I = 0; I != NumElems; ++I) {
+ uint32_t CodePoint = I < N ? SL->getCodeUnit(I) : 0;
----------------
Aren't `N` and `NumElems` guaranteed to have the same value here? Both are
derived from `SL`. The code seems to be written with the expectation that
`NumElems` corresponds to the number of elements to be iniitialized in the
target array.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1100
+ for (size_t I = 0; I != NumElems; ++I) {
+ uint32_t CodePoint = I < N ? SL->getCodeUnit(I) : 0;
+ // TODO(Perf): 0 is implicit; we can just stop iterating at that point.
----------------
`CodePoint` should be `CodeUnit` here. `char` and friends all hold code units
(in some cases, those code units may also constitute a code point).
================
Comment at: clang/test/AST/Interp/literals.cpp:354-359
+ constexpr char foo[12] = "abc";
+ static_assert(foo[0] == 'a', "");
+ static_assert(foo[1] == 'b', "");
+ static_assert(foo[2] == 'c', "");
+ static_assert(foo[3] == 0, "");
+ static_assert(foo[11] == 0, "");
----------------
aaron.ballman wrote:
> I'd like to see some tests for the other encodings, as well as a test with
> embedded null characters in the literal.
>
> Testing a string literal that's longer than the array is something we should
> think about. That code is ill-formed in C++, so I don't think we can add a
> test for it yet, but it's only a warning in C.
I agree with Aaron's requests. Please also extend the test to include a `char`
element that would be negative for a `signed` 8-bit `char`. Something like:
constexpr char foo[12] = "abc\xff";
...
#if defined(__CHAR_UNSIGNED__) || __CHAR_BIT__ > 8
static_assert(foo[3] == 255, "");
#else
static_assert(foo[3] == -1, "");
#endif
A couple of more tests to add:
- One where the string literal has the same length (including the implicit
terminator) as the array; to ensure that the implicit terminator is properly
accounted for.
- One where the target array size is deduced from the string literal; to ensure
there are no dependencies on an explicit array size.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137488/new/
https://reviews.llvm.org/D137488
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
