[libcxx-commits] [PATCH] D114001: [libc++][format] Adds formatter floating-point.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/formatter.h:211
+  _LIBCPP_ASSERT(__first <= __last, "Not a valid range");
+  _LIBCPP_ASSERT(__num_trailing_zeros > 0, "The overload not writing trailing zeros should have been used");
+
----------------
Quuxplusone wrote:
> `_LIBCPP_ASSERT(__num_trailing_zeros > 0, "The overload not writing trailing zeros should have been used");`
> I haven't looked really closely at the logic around this, but it feels very weird that we have two different overloads — one taking `__num_trailing_zeros` but internally checking that it's never zero, and one //not// taking `__num_trailing_zeros` and internally assuming that it's always zero. Then, in the caller (new line 603), we have `if (__num_trailing_zeros) /*call the first overload*/ else /*call the second overload*/`. Couldn't we just have one overload, and no precondition, and do the `if` test //inside// the callee?
> 
> Also, this smells very much like "When Should You Give Two Things the Same Name?". Why do you need the two overloads to //both// be named `__write`, so that accidentally omitting an argument will do the wrong thing? Could you simply name them `__write` and `__write_with_trailing_zeros` instead?
> If so, then //even if you keep everything else the same//, your precondition-failure message can use the greppable and unambiguous name "`__write_with_trailing_zeros`" in place of the verbose epithet "The overload not writing trailing zeros".
I prefer to keep it as is, but I'll adjust the assertion message in the next revision/before landing.

(I'm looking at some size optimizations and I expect some of this code will be changed in the future.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114001