[libcxx-commits] [PATCH] D146294: [libcxx] Fix crash in std::stringstream with payload >= INT_MAX

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D146294#4324132 <https://reviews.llvm.org/D146294#4324132>, @azat wrote:

> Friendly ping, can someone review this?

Sorry I missed the updates to this patch.

I'm happy with the code, I'm only a bit concerned with the test.



================
Comment at: libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp:19
+int main(int, char**) {
+#if __SIZE_WIDTH__ == 64
+  std::stringstream ss;
----------------
Why is this needed? Can't you just compare that an int is 32-bit?

I ideally I would rather make the test unsupported on platform that are not used. Otherwise this test gives a false sense of doing something.

Maybe we should add a feature in `libcxx/utils/libcxx/test/features.py` Something like `can-test-gcount` Then we only enable it on 64-bit platforms where int is a 32-bit value. WDYT?


================
Comment at: libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp:23
+
+  for (size_t i = 0; i < (2ULL << 30) - payload.size(); i += payload.size()) {
+    assert(ss.tellp() != -1);
----------------
Why this loop? Wouldn't it be easier to:
- create a string with `INT_MAX - 1` elements
- write the string and validate (INT_MAX - 1)
- write 1 character and validate (INT_MAX)
- write 1 character and validate (INT_MAX + 1)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146294