[libcxx-commits] [PATCH] D96477: [libcxx] adds remaining callable concepts

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb planned changes to this revision.
cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/callable/equiv.compile.pass.cpp:88
+    /// ModelsEquivalenceRelation(Relation::Greater, 0, 0);
+    /// ModelsEquivalenceRelation(Relation::Greater, 0.0, 0);
+    ModelsEquivalenceRelation(Relation::MaybeRelation(), 0, 0);
----------------
zoecarver wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > I don't think it's useful to keep these as comments.
> > > What is your understanding of the requirements on libc++ as a conforming implementation of C++20? Is libc++ //permitted// to reject these lines if they were to be uncommented?
> > > If we're not permitted to reject them, then we must accept them, and so you might as well uncomment them permanently. OTOH, if they're supposed to be some sort of "library IFNDR," such that a conforming library might reject them, then it makes sense not to uncomment them (but in that case I'd just delete these lines permanently).
> > They're INFDR, and I want to document that they're not forgotten. See http://eel.is/c++draft/res.on.requirements.
> There's no other place that we'd say, "here are all the ways we can get this to compile with incorrect semantics." Your comment with `Relation::Greater` doesn't make sense because it's not reflexive, transitive, or symmetric. If we had some way to test that `Relation::Greater` //didn't// model `equivalence_relation`, that would make sense, but, because this is purely semantic, we can't do that. 
> 
> I don't think it's a bad idea to clarify that these relations haven't been forgotten and have instead been explicitly omitted. But, I think the comment could be more clear/concise about it. I think you should either put a comment at the top (like you have) stating that these relations are explicitly not tested, or update the comments to say `NotEquivalenceRelation` or `!std::equivalence_relation` so it's clear that they're semantically not equivalence relations. 
@zoecarver I'm confused about whether or not you're saying what's present is sufficient, or if you'd like something else.


================
Comment at: libcxx/test/std/concepts/callable/functions.h:27-28
+struct Bool {
+  int Value = false;
+  constexpr operator bool() const noexcept { return Value; }
+};
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > Quuxplusone wrote:
> > > > cjdb wrote:
> > > > > Quuxplusone wrote:
> > > > > > 
> > > > > No, this is definitely correct as-is.
> > > > There is no //conceivable// library bug that could cause us to treat `int Value = false;` and `int Value = 0;` any differently.
> > > > Also, you don't want to return `int Value` from `operator bool`; you want to return a `bool`.
> > > > 
> > > > Maybe you meant to write `bool Value = false;` instead of `int Value = false;`, is that the issue?
> > > @Quuxplusone why do you want to remove noexcept? 
> > Well, I'm always on the side of "keep it simple/short." Nothing in C++ cares about whether your conversion-to-`bool` operator is noexcept, so there's no reason to make it noexcept. (Also, it'll rarely if ever be noexcept in real-world code, so one could argue that testing shorter code is also testing //more realistic// code.)
> Fair enough. FWIW I don't feel strongly one way or another about the noexcept. 
> There is no conceivable library bug that could cause us to treat `int Value = false;` and `int Value = 0;` any differently.

That appears to have been integrated (I don't recall whether I meant `bool Value` or `int Value`, but the point is moot either way).

I was referring to the suggested addition of `explicit`, which is not desirable, since `__boolean_testable` requires `Bool` to be both implicitly and explicitly convertible.

> Well, I'm always on the side of "keep it simple/short."

I don't think `noexcept` changes this in either direction.

> Also, it'll rarely if ever be noexcept in real-world code, so one could argue that testing shorter code is also testing more realistic code.

Citation needed. I certainly apply `noexcept` to everything that's conceivable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477