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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added inline comments.


================
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.
----------------
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. :)


================
Comment at: libcxx/include/format:581
 }
 #endif
 
----------------
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>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113831