[libcxx-commits] [PATCH] D113831: [libc++][NFC] Move format_to_n_result.

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
Comment at: libcxx/include/__format/format_to_n_result.h:24-27
+// TODO FMT Remove this once we require compilers with proper C++20 support.
+// If the compiler has no concepts support, the format header will be disabled.
+// Without concepts support enable_if needs to be used and that too much effort
+// to support compilers with partial C++20 support.
----------------
Quuxplusone wrote:
> FWIW, I think this TODO is obvious. I'm not worried that we'll forget to eliminate `_LIBCPP_HAS_NO_CONCEPTS` once that becomes possible. :)
True but this is the comment I've used before and try to keep it consistent ;-)
I would really love to get rid of this TODO...


================
Comment at: libcxx/include/format:581
 }
 #endif
 
----------------
Quuxplusone wrote:
> LGTM, ship it (once CI passes).
> But FWIW, for any header where we're doing the "granular" thing, I would strongly prefer that the top-level header contain //nothing// but `#include` directives (and boilerplate). That is, I think you should follow up, as soon as you get the cycles, by moving `format`, `formatted_size`, etc., into their own headers.
> 
> I also don't understand why you're splitting up `struct format_to_n_result` from the `format_to_n` function itself; shouldn't they go together into `<__format/format_to_n.h>`?
I'm not going to mvoe the format functions for now. I'm happy to add a TODO FMT for now. The reason not to do it now it to avoid merge conflicts with the P2216 patches under review now. (I've some more patches not yet under review, so I strongly prefer to wait with this until it won't cause needless rebase issues.)

I moved only the struct to a new header since it will be used in the `__format/buffer.h` header and in `format`. This is part of the PoC P2216 patch D112361. Currently I'm reworking that patch to patches in that stack to have small modular patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113831