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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review comments!



================
Comment at: libcxx/include/format:630-631
+  default:
+    __throw_format_error(
+        "The replacement field arg-id should terminate at a ':' or '}'");
+  }
----------------
Quuxplusone wrote:
> 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.)
This change is automatically done by clang-format since I've still limited it to 80 characters instead of 120 as our official standard is. This since parts of this series were written before the limit was adjusted. After all outstanding patches are merged I want to apply our real coding style with clang-format, this will fix it. I don't want to do that now, since it will result in merge conflicts.

I don't mind using `_VSTD::__throw_format_error` but currently all calls to `__throw_xxx` functions use this style. So when I change it here I want to create a separate patch to do the rest of our code base. @ldionne what's your preference?


================
Comment at: libcxx/include/format:648
+  if (__begin == __end || *__begin != _CharT('}'))
+    __throw_format_error("The replacement field misses the terminating '}'");
+
----------------
Quuxplusone wrote:
> /misses the/is missing a/
I changed it to 'misses a', this is more in line with the other messages in this file.


================
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");
+
----------------
Quuxplusone wrote:
> - 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`.
Again this is what clang-format does. When we don't like it we should adjust its settings to the format we want.
Originally this function was used in more places, but addressing other review comments reduced the number of use cases.
With only two users it indeed seems better to inline them. This allows for a small code improvement at the second call site.


================
Comment at: libcxx/include/format:692-693
+      __begin = __next(__begin, __end);
+      if (*__begin != _CharT('}')) [[unlikely]]
+        __throw_format_error("The format string contains an unmatched '}'");
+
----------------
Quuxplusone wrote:
> 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.)
I wasn't aware compilers already optimize the throwing path as unlikely. I tested it on compiler explorer and it indeed seems to be the case.
I like `[[nodiscard]]` for two reasons:
* It avoids accidental bugs by not evaluating.
* Documents the intended usage.

Even if a function isn't used in `constexpr` context it can be useful. I've used it during testing and suddenly UB became a compilation error.

So I don't consider `[[nodiscard]]`, `noexcept` and `constexpr` as clutter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664