[libcxx-commits] [PATCH] D121123: [libc++] Add a way to enable lightweight assertions independent of the debug mode

Tue Mar 8 12:22:28 PST 2022

ldionne marked an inline comment as done.
ldionne added inline comments.

================
Comment at: libcxx/benchmarks/algorithms.bench.cpp:154
 void sortValues(T& V, Order O) {
-  assert(std::is_sorted(V.begin(), V.end()));
   switch (O) {
----------------
Mordante wrote:
> Is this intended or for testing. When intended please commit it separately.
Agreed -- see D121244.

================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
Mordante wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > Mordante wrote:
> > > > > ldionne wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > ldionne wrote:
> > > > > > > > philnik wrote:
> > > > > > > > > ldionne wrote:
> > > > > > > > > > I am not very happy about requiring users to include an internal header to customize the assertion handler. IMO that's kind of hacky. I thought about using `<version>` instead, but that would require `<version>` to include `<__assert>`, and unfortunately that's a no-go because `<__assert>` needs to include `<iosfwd>` right now (due to the legacy debug mode support).
> > > > > > > > > Why not put it in `<cassert>`? That would make the most sense IMO.
> > > > > > > > I agree it makes some sense, however we still have the problem that we need to include `<iosfwd>` from this header.
> > > > > > > > 
> > > > > > > > In other words, I don't think we can easily change this part of the user-facing API unless we agree to drop `<iosfwd>` from `<__assert>` (and in that case we can even include it from e.g. `<__config>`).
> > > > > > > Tangent FWIW: it seems like we could use a `<__stringfwd>` helper header.
> > > > > > I agree, and it's useful for e.g. D116950 anyways.
> > > > > I think when we need an internal header it would be a good idea to add a new header specifically for that purpose.
> > > > > `__libcpp_assertion_hander` for example. That header can then include `__assert`, but that way we can change our implementation without breaking user code.
> > > > +1 Mordante's idea of `<__libcpp_assertion_handler>`, except that I worry that "one header per entry point" is too fine a level of granularity. Like maybe it should be `<__libcpp_assert>` and pull in all the stuff relevant to assertion handlers (`__libcpp_assertion_handler`, `__libcpp_assertion_handler_type`, `__libcpp_set_assertion_handler`...). And then that's just `<__assert>` (i.e. what Louis has done) but with a `libcpp` on the front. Which might still be a nice touch.
> > > > 
> > > > If we were to rename `__assert` to `__libcpp_assert`, would that hurt any users? (No, because `__assert` hasn't shipped yet?)
> > > > If we were to rename it, what would be connoted by the symmetry between `__libcpp_assert` and `__libcpp_version`? Would that be a good thing or a bad thing? (Is `__libcpp_version` even shipped? I have never bothered to understand its job.)
> > > > 
> > > > Is this a job for a subdirectory, like `<libcpp/assert.h>` or `<ext/libcpp_assertions.h>` or something?
> > > I'm not sure I understand the request here. We already have a header that includes "everything related to assertions", and the name of that header is `<__assert>`. We can discuss changing the name of that header, no problem, but I'm not sure I understand your request if it's not just to rename the header. What would `< __libcpp_assertion_hander>` contain?
> > FWIW, I interpreted @Mordante's comment as a reply to @philnik's suggestion; i.e. my interpretation of this thread has been [all words are my own and quite possibly //not// what people were actually intending to convey]:
> > 
> > L: "I'm not thrilled about making people include `<__assert>`, because it isn't very discoverable, and it might encourage them to include other detail headers like `<__string>`, which we do //not// want them to."
> > P: "How about `<cassert>` then? it's more discoverable, and obviously user-facing."
> > M: "How about `<__libcpp_assertion_handler>`? it's named exactly the same as the facility they must already know about because they're trying to use it, and maybe the `libcpp` prefix indicates that it's more implementation-specific and also more special than `<__string>` and so on."
> > Q: "Yeah, not `<cassert>`, and I like the `libcpp` prefix idea. Or at least some sort of prefix that separates this thing from `<__string>` and `<__hash_table>`. How about put it in a subdirectory?"
> > 
> > @ldionne wrote:
> > > I'm not sure I understand your request if it's not just to rename the header.
> > 
> > My interpretation was that it //did// boil down to just a request to rename the header, but I could be wrong.
> Correct. Except I wasn't aware `__debug` was recent. Hence my proposal to have a separate header to include `__debug`. When `__debug` is still new renaming seems better than adding an additional header.
I'm OK with having a header named `__libcpp_assertion_handler` or ideally `__libcpp_assert` for users to include, instead of `<__assert>`. However, I do not want to merge `__debug` and `__assert` (however it ends up being named), specifically because I want to keep a clear distinction between the notion of assertions and the notion of a debugging mode. This lack of distinction is what has been muddying the waters for so long, and specifically what I'm trying to fix.

Most of our code wants the ability to add assertions for preconditions, UB and stuff like that. Only a tiny fraction of our code cares about the full debug mode -- basically only containers and a couple of algorithms.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123