[libcxx-commits] [PATCH] D121530: [libc++][format] Implement format-string.

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review! I've addressed some of the smaller comments, will look at the others tomorrow.



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:260
    */
-  constexpr bool __has_width_field() const noexcept {
-    return __width_as_arg || __width;
-  }
+  constexpr bool __width_needs_substitution() const noexcept { return __width_as_arg; }
 
----------------
vitaut wrote:
> Why make it public? Isn't it only called by subclasses?
Not anymore, `__compile_time_validate_argument` calls the function too, there it needs to be public.


================
Comment at: libcxx/include/format:217
+template <class _CharT>
+class _LIBCPP_TEMPLATE_VIS __compile_time_handle {
+public:
----------------
vitaut wrote:
> What is this class for? A comment would be nice.
I added comment. Basically when a handle is used the code needs to use the `parse` function of the formatter associated with the type `_Tp`.


================
Comment at: libcxx/include/format:302
+  case __arg_t::__long_long:
+#  ifndef _LIBCPP_HAS_NO_INT128
+  case __arg_t::__i128:
----------------
vitaut wrote:
> Is this ifdef needed? In principle you could always define `__i128` and `__u128` but only use it if `_LIBCPP_HAS_NO_INT128` is set. This might be beneficial for ABI compatibility and simplify some code like the one here.
Good point. Here it doesn't have an ABI impact. Based on earlier review comments by you in another patch I made these field unconditionally in the enum. So I can use them here unconditionally.


================
Comment at: libcxx/include/format:569-573
+#    ifdef _WIN32
+          __basic_format_string<char, type_identity_t<_Args>...> __fmt,
+#    else
+          __format_string<_Args...> __fmt,
+#    endif
----------------
vitaut wrote:
> Can we avoid these ugly ifdefs? Why not rename `__format_string` to `__format_string_t` or similar?
I originally added them due to odd errors on Windows. Later I figured out it wasn't a bug of the Windows compilers but that `__format_string` is a SAL macro on Windows. (Not the first time I've been bitten by SAL macros.)

For now I prefer to keep them since I expect these become `format_string` during the plenary in July.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530