[libcxx-commits] [PATCH] D99461: [libcxx] adds `std::indirectly_readable_traits` to <iterator>

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp:66
+  return result;
+}
+
----------------
zoecarver wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > This function should use regular `assert` and `return true;` at the end.
> > See https://reviews.llvm.org/D99141#inline-935428.
> I'm not sure what we get from using regular assert and returning true. We made this the standard quo for //updating// algorithms, etc. to be constexpr, because we wanted to preserve the existing test coverage in a constexpr context (i.e., both at runtime and during constexpr evaluation the same tests would be run), but that doesn't really apply here. These tests are only run in a constexpr context. 
> 
> In fact, I think it would actually be worse to use regular assert because that will give a confusing error and likely be much slower. (Regular assert will result in a "is not an integral constant" error whereas the static_assert will give the full stack trace, I think.)
Even if it doesn't give a full stack trace, "is false" is much clearer than "is not an integral constant", since the latter could be issued for a number of reasons.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99461