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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/format:692-693
+      __begin = __next(__begin, __end);
+      if (*__begin != _CharT('}')) [[unlikely]]
+        __throw_format_error("The format string contains an unmatched '}'");
+
----------------
Mordante wrote:
> 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.
I had a talk with @ldionne and he also prefers to remove the extra `[[nodiscard]]`s. He mention they've also been removed in the ranges code, which previously used the same style as I did here. So we agreed to remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664