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

Tue Mar 8 11:24:33 PST 2022

Mordante added a comment.

I've given this review some more thoughts and I'm not sure whether this is the best approach. I've send you a message to look at a slightly different approach.

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

================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123