[libcxx-commits] [PATCH] D110738: [libc++] [P1614] Implement the first half of [cmp.alg]: strong_order, weak_order, partial_order.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/docs/Status/Cxx20Issues.csv:253
 "`3329 <https://wg21.link/LWG3329>`__","``totally_ordered_with``\  both directly and indirectly requires ``common_reference_with``\ ","Prague","|Complete|","13.0"
-"`3330 <https://wg21.link/LWG3330>`__","Include ``<compare>``\  from most library headers","Prague","","","|spaceship|"
+"`3330 <https://wg21.link/LWG3330>`__","Include ``<compare>``\  from most library headers","Prague","|Complete|","13.0","|spaceship|"
 "`3331 <https://wg21.link/LWG3331>`__","Define ``totally_ordered/_with``\  in terms of ``partially-ordered-with``\ ","Prague","|Complete|","13.0"
----------------
ldionne wrote:
> Do you mean 14.0 or 13.0 here?
I believe I mean 13. This was D99309.


================
Comment at: libcxx/include/__compare/partial_order.h:38
+        {
+            return             partial_ordering(partial_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f)));
+        }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > ldionne wrote:
> > > > Mordante wrote:
> > > > > Quuxplusone wrote:
> > > > > > Mordante wrote:
> > > > > > > I like this manually formatting a lot! Would it make sense to guard this code against clang-format, just in case we want to enable it for the entire libc++ in the future?
> > > > > > This is our standard formatting now for "you must write it three times" functions; see D108144. (Which came out of discussion on some other review.)
> > > > > > We guard it against clang-format in the same way as we guard the entire rest of libc++: don't run clang-format on these files, and if someone accidentally (or just naively) does, then we hope to catch it in code review.
> > > > > > (We could start wrapping whole files in `clang-format off` comments, but: That's an O(n) solution to an O(1) problem. And, it works only because `clang-format` is a monoculture. If anyone ever writes a second C++ formatter, we'd have to guard against //that// one, too. To really ensure that nobody can mess with our aesthetics, we'd put in some sort of technological "do-not-sed" and "do-not-awk" protection... I'm having flashbacks to DeCSS here... ;))
> > > > > Fair point. I recall some of these discussions and the nice link you posted about a lightning talk regarding this issue.
> > > > As a side note, usually we do this:
> > > > 
> > > > ```
> > > > noexcept(noexcept(EXPRESSION))
> > > > -> decltype(      EXPRESSION)
> > > > { return          EXPRESSION; }
> > > > 
> > > > ```
> > > Ping on this - please format consistently with the other places where we do this. It helps for grepping.
> > The difference was just the extra spaces I put around `noexcept(noexcept( E ))` and `decltype( E )`, right? Is this the way you want it now?
> Sorry, should have made it clearer, but the difference is the positioning of braces. This seems like a nitpick, but I've mass-modified these expressions in the past and having consistent formatting really helps.
Aha. I didn't notice the braces. Fixed now, AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738