[libcxx-commits] [PATCH] D155349: [libc++][hardening] Categorize most assertions inside the container classes.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! In general LGMT. Some remarks regarding the naming; I know it seems a bit like bike shedding, but IMO we should have good names. Once we expose these names to users it will be a lot harder to change them. I feel some names are quite "short" and make it hard to use similar tests in the future with a consistent name.



================
Comment at: libcxx/include/__config:239
+// - `_LIBCPP_ASSERT_VALID_INPUT_RANGE` -- checks that ranges (whether expressed as an iterator pair, an iterator and
+//   a count, or a `std::range`) given as input to library functions are valid:
+//   - the sentinel is reachable from the begin iterator;
----------------
This misses `iterator and sentinel`.


================
Comment at: libcxx/include/__config:247
+//   purposes of this check. This check only applies to member functions of containers, not any access made purely via
+//   iterators.
+//
----------------
I see the point of calling everything a container and making them a one element container. I wonder whether the name `_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS` would be better.


================
Comment at: libcxx/include/__config:252
+//
+// - `_LIBCPP_ASSERT_VALID_ALLOCATOR` -- checks any operations that merge or swap containers to make sure the containers
+//   have compatible allocators.
----------------
I expect in the future there might be other tests on allocators. So I think using a longer name leaves more space for future naming.


================
Comment at: libcxx/include/__config:255
+//
+// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
+//   user input.
----------------
Likewise leaving room for other internal hardining options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155349