[libcxx-commits] [PATCH] D151637: DRAFT: hardening "interface"

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__config:248
+//
+// #define _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS 1
+
----------------
ldionne wrote:
> Since this affects both hardening and "debug" checks, I don't think `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS` is the right name. Is this different from `_LIBCPP_ENABLE_ASSERTIONS`?
> 
> Answer: The difference between this macro and `_LIBCPP_ENABLE_ASSERTIONS` is that the latter also disables some currently-uncategorized assertions.
> 
> IMO it's not worth introducing another public knob for this, I think people don't need that much granularity. I can't think of anyone wanting to disable hardening and debug assertions while somehow keeping all the "uncategorized" assertions.
https://reviews.llvm.org/D153816 (`s/_LIBCPP_ASSERT/_LIBCPP_ASSERT_UNCATEGORIZED`).


================
Comment at: libcxx/include/__config:331
+
+#  define _LIBCPP_ASSERT_CHECK_INVALID_RANGE(...) ((void)0)
+
----------------
ldionne wrote:
> var-const wrote:
> > I copied this from existing macros. Is there a reason to prefer this form to just `/*nothing*/`?
> I think it's to make this valid to use in an expression. I have a test that `_LIBCPP_ASSERT` can be used in an expression, and we should have similar tests for new assertion macros being introduced (possibly add to the same test).
Do we need to add those tests considering that we deliberately define the new assertions to expand to `_LIBCPP_ASSERT` which is already thoroughly tested in this regard?


================
Comment at: libcxx/include/span:241
+      _LIBCPP_ASSERT_CHECK_BAD_CONTAINER_ACCESS(_Extent == __count, "span::span(Iter, Sent)");
+      _LIBCPP_ASSERT_CHECK_INVALID_RANGE(__first, __count, "span::span(Iter, size_type)");
     }
----------------
ldionne wrote:
> I find this potentially confusing. The name `_LIBCPP_ASSERT_CHECK_INVALID_RANGE` makes it look as if we're checking that the range is invalid, when we're trying to do the reverse. Should this be called `_LIBCPP_ASSERT_CHECK_VALID_RANGE` instead (and similarly for other types of checks)?
I think it depends on how the reader sees the purpose of the check. I'm looking at it from the perspective of "which condition will cause the assertion to trigger?" (and more broadly, "what kind of bad behavior are we providing guarantees against?"). Or to put it simpler, I'm reading this kinda like "abort if the range is invalid", which I find slightly more natural than "make sure the range is valid and proceed".

I don't feel too strongly about this and can be convinced to change. One argument in favor of "check valid range" is that it follows the semantics of "assert" (which fires if the condition is false, not true).

EDIT: after some thinking, I went ahead with the renaming as suggested in this comment.


================
Comment at: libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp:117
   friend bool operator==(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ == y.it_; }
-  friend bool operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
+  friend auto operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
 };
----------------
ldionne wrote:
> var-const wrote:
> > Looks like this operator wasn't invoked before.
> Let's land this as a NFC before this patch. Actually, is there a reason for defining it at all if it's not used?
Done. It's used by some of the hardening checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151637