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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__format/format_string.h:130
+      __throw_format_error(
+          "The numeric values of the format-spec is too large");
+
----------------
s/values/value/
and please don't line-break


================
Comment at: libcxx/include/format:452
+        __value > numeric_limits<_To>::max())
+      __throw_format_error("128 bit value is outside of implemented range");
+
----------------
s/128 bit/128-bit/g


================
Comment at: libcxx/include/format:630-631
+  default:
+    __throw_format_error(
+        "The replacement field arg-id should terminate at a ':' or '}'");
+  }
----------------
Style tip: When you break the line like this, it makes it harder for a maintainer to `git grep __throw_format_error.*terminate` when they're trying to track down where a given error was thrown. (Although line-breaking in the middle of a string is of course infinitely worse!)
In this case there's no reason to line-break at all; just join lines 630 and 631. Ideally, every single time you throw a format_error, it'll be on a line of the form
```
[ ]*__throw_format_error(".*");
```

(Tangent: I would not object if you made it `_VSTD::__throw_format_error` throughout. You're right that the string literal won't trigger ADL, so ADL-proofing isn't necessary; but it might help visually distinguish this free function from a member function.)


================
Comment at: libcxx/include/format:648
+  if (__begin == __end || *__begin != _CharT('}'))
+    __throw_format_error("The replacement field misses the terminating '}'");
+
----------------
/misses the/is missing a/


================
Comment at: libcxx/include/format:658-659
+  if (__begin == __end) [[unlikely]]
+    __throw_format_error("The format string terminates while parsing a "
+                         "replacement field or an escape sequence");
+
----------------
- Don't break in the middle of this error message.
- This function is called in only two places; I recommend inlining it in those places. Then you can improve the message by customizing it to the specific situation being parsed.
- If you do keep it as a free function (which I don't recommend), please `_VSTD::` its calls, and consider giving it a name that (1) doesn't look so much like the uglified version of `std::next`, (2) involves the word `format`.


================
Comment at: libcxx/include/format:692-693
+      __begin = __next(__begin, __end);
+      if (*__begin != _CharT('}')) [[unlikely]]
+        __throw_format_error("The format string contains an unmatched '}'");
+
----------------
Lines 692, 677, 657: I question the usefulness of these `[[unlikely]]` attributes. Compilers are generally very good at determining that the throwing path is the unlikely path, already; and this seems like cluttering up our code for no practical purpose.
(Ditto `[[nodiscard]]` on non-user-visible functions, such as on lines 45, 63, 70, 80; ditto `noexcept` on non-user-visible functions that are never evaluated for noexceptness; ditto `constexpr` on non-user-visible-functions that are never called in a constexpr context.)


================
Comment at: libcxx/include/format:687
+        __throw_format_error(
+            "The format string contains an invalid escape sequence");
+
----------------
Mordante wrote:
> vitaut wrote:
> > I would call it something like "unmatched '}'" rather than "an invalid escape sequence" because we don't know whether the intent was to have an escape sequence.
> I like your reasoning, I've changed it.
s/charater/character/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664