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; ---------------- tahonermann wrote: > 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. I see the change to now use the minimum of `SL->getLength()` and `CAT->getSize().getZExtValue()`. Based on https://godbolt.org/z/5sTWExTac this looks to be unnecessary. When a string literal is used as an array initializer, it appears that the type of the string literal is adjusted to match the size of the array being initialized. I suggest using only `CAT->getSize().getZExtValue()` and adding a comment that this code depends on that adjustment. ================ 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, ""); ---------------- tahonermann wrote: > 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. These cases all look to have been added now. Thank you! 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