[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 8 inline comments as done.
Mordante added a comment.

I'm still working on the removal of the `999.999.999` in the underlaying patches. It causes quite a bit of changes.



================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:13-17
+// For each charT, for each cv-unqualified arithmetic type ArithmeticT other
+// than char, wchar_t, char8_t, char16_t, or char32_t, a specialization
+// - a specialization template<> struct formatter<ArithmeticT, charT>;
+// Implemants ArithmeticT is
+// - bool
----------------
Quuxplusone wrote:
> vitaut wrote:
> > Mordante wrote:
> > > vitaut wrote:
> > > > I'm not sure how to parse this comment. Is something missing here?
> > > This is part of the standard wording of the part tested.
> > The confusing to me part is that "a specialization" appears twice. I suggest replacing `a specialization template<> struct formatter<ArithmeticT, charT>;` with `template<> struct formatter<ArithmeticT, charT>;`
> Not to mention, "Implemants" on line 16 definitely isn't standard wording, and line 17 seems to be starting a bulleted list with only one item in it. I think this comment is meant to refer to https://eel.is/c++draft/format.formatter.spec#2.3 and so it should say:
> 
> ```
> // [format.formatter.spec]:
> // Each header that declares the template `formatter` provides the following enabled specializations:
> // For each `charT`, for each cv-unqualified arithmetic type `ArithmeticT` other than
> // char, wchar_­t, char8_­t, char16_­t, or char32_­t, a specialization
> //    template<> struct formatter<ArithmeticT, charT>
> //
> // This file tests with `ArithmeticT=bool`, for each valid `charT`.
> ```
I overlooked the double specialization. I'll adjust the text.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:59-62
+    const CharT* (F::*parse)(PC&) = &F::parse;
+    assert(parse);
+    Out (F::*format)(bool, C&) = &F::format;
+    assert(format);
----------------
vitaut wrote:
> Quuxplusone wrote:
> > These asserts seem non-standard: I mean, I would by default assume there's no legal guarantee that `&F::parse` compiles at all. `parse` might be a template or overload set.
> > @vitaut , what's your understanding and/or intent here? Are user-programmers supposed to be able to take member pointers to the `parse` and `format` callables like this? (I hope not!)
> IIRC users have no right to rely on specific signatures of functions in `std` so I would recommend deleting these over-specified asserts.
I'll remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664