[libcxx-commits] [PATCH] D103368: [libc++][format] Adds parser std-format-spec.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut requested changes to this revision.
vitaut added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:132
+  // character. Validate what fmt and MSVC have implemented.
+  _CharT __fill{_CharT(' ')};
+
----------------
This should really be a grapheme cluster but {fmt} uses a code point at the moment (grapheme clusterization will be added later). In any case this should be a range (e.g. string_view) and not a single code unit.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:138
+    _LIBCPP_ASSERT(__begin != __end, "Precondition failure");
+    if (__begin + 1 != __end) {
+      if (__detail::__parse_alignment(*(__begin + 1), __flags)) {
----------------
This will have to change for the same reason.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:184-185
+    ++__begin;
+    if (__begin == __end)
+      __throw_format_error("End of input while parsing format-spec sign");
+
----------------
This is incorrect because format specs can consist of a single sign specifier. Same for other "End of input" errors.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:196-201
+    switch (*__begin) {
+    case _CharT('+'):
+    case _CharT('-'):
+    case _CharT(' '):
+      __throw_format_error("Format-spec sign not allowed");
+    }
----------------
I don't think you need to have this check because it will be caught elsewhere anyway. Format specifiers are type-specific and it generally makes more sense to say what is supported rather than what is not. Same for other `__parser_no_*` cases.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:277-279
+  if (*__r.first == _CharT(':'))
+    __throw_format_error(
+        "Format-spec arg-id shouldn't contain its own format-spec");
----------------
This is redundant and only adds an overhead without substantially improving diagnostics. The error message is not particularly correct either.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:307-308
+              static_cast<_CT>(__format::__number_max))
+            __throw_format_error("Format-spec arg-id replacement "
+                                 "exceeds maximum supported value");
+          return __arg;
----------------
It might be better to just return a large value and let the error be reported elsewhere (when the ID is actually used). It's both more efficient and gives better diagnostic without referring to internal magic limits.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:321
+  // for the width. It would be possible to store it in an int32_t and use
+  // negative values for a real with and the other values for an arg-id. This
+  // would be possible since 0 isn't a valid width. However 0 is a valid
----------------
with -> width


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:373
+public:
+  uint32_t __precision : 31 {__format::__number_max};
+  uint32_t __precision_as_arg : 1 {0};
----------------
Note that any nonnegative `int32_t` value is a valid precision so I'm not sure `__number_max` is OK here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103368