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

Wed Mar 9 09:26:09 PST 2022

Mordante added a comment.

In general LGTM, I want to wait with approval after the CI is green. (I also like to see whether you think the weak-def version is a better solution.)

================
Comment at: libcxx/docs/UsingLibcxx.rst:170
+  // for simplicity in this example.
+  inline [[noreturn]] void HelloWorldAssertionHandler(std::__libcpp_assertion_info info) {
+      std::printf("Assertion %s failed at %s:%d, more info: %s",
----------------
The current version is ill-formed.

================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
ldionne wrote:
> 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.
Sorry my bad I meant `__assert` instead of `__debug`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123