[libcxx-commits] [PATCH] D136765: [ASan][libcxx] Annotating std::vector with all allocators

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Considering that this won't be cherry-picked back to `release/16.x` (right?), then whether we use `_LIBCPP_CLANG_VER >= 1600` or `_LIBCPP_CLANG_VER >= 1700` is irrelevant for all practical purposes, since libc++ gets released in sync with Clang and the next release this will get in will have Clang 17. I do agree however that if someone (in this case Chrome) is using a not-yet-released snapshot of `trunk` that happens to have `_LIBCPP_CLANG_VER >= 1600` yet isn't something we've released as LLVM 16, then it's probably not reasonable for us to try to support that. Imagine coming up with that policy, I think it would mean that we can't ever rely on `_LIBCPP_CLANG_VER >= 1600` during the libc++ 16 release, which would be pretty unfortunate.

So I have no strong opinion, either way is fine with me (even though the status quo is kind of unfortunate for Chrome until they bump their Clang version).



================
Comment at: libcxx/test/libcxx/containers/sequences/vector/asan.pass.cpp:45
 
-    {
-        typedef cpp17_input_iterator<int*> MyInputIter;
-        // Sould not trigger ASan.
-        std::vector<int> v;
-        v.reserve(1);
-        int i[] = {42};
-        v.insert(v.begin(), MyInputIter(i), MyInputIter(i + 1));
-        assert(v[0] == 42);
-        assert(is_contiguous_container_asan_correct(v));
-    }
+#if TEST_STD_VER >= 11 && TEST_CLANG_VER >= 1600
+  // TODO LLVM18: Remove the special-casing
----------------
Since we're mostly interested in having good coverage for the Clang >= 16 behavior anyway, I would instead suggest keeping this test but removing the `#if TEST_STD_VER >= 11 && TEST_CLANG_VER < 1600` test above. I don't really see much value in keeping it around, unless I missed something.


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/no_asan.pass.cpp:13
+
+// Test based on: https://bugs.chromium.org/p/chromium/issues/detail?id=1419798#c5
+
----------------
Can you please add an english description of what this test does? It's also nice to have the link, however the test should ideally be understandable just by reading it (if e.g. the link goes down or something).


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/no_asan.pass.cpp:57
+
+int main() {
+  using V = std::vector<int, user_allocator<int>>;
----------------
Nitpick, but this is needed for freestanding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765