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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver 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);
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477