[libcxx-commits] [PATCH] D103357: [libc++][format] Add __format_arg_store.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/format_context.h:59
+    basic_format_args<basic_format_context<_OutIt, _CharT>> __args,
+    optional<_VSTD::locale>&& __loc = nullopt) {
+  return basic_format_context(_VSTD::move(__out_it), __args,
----------------
Quuxplusone wrote:
> Re your comment on D106567 — It would be cleaner here to write "one function declaration per function signature," i.e. get rid of the defaulted function argument ( https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil ), and then I bet you wouldn't run into any modules issues with the 2-argument overload (because it wouldn't mention `optional` in its signature) nor the 3-argument overload (because it wouldn't mention `optional` in its signature, either). The next step would be to give the two things not-the-same name.
> I actually talk about the "ooh, I'll take an optional that defaults to nullopt!" antipattern in //[Back to Basics: Algebraic Data Types](https://www.youtube.com/watch?v=OJzmWqCCZaM&t=2792s)// (CppCon 2020), and obviously I've talked about When to Give Two Things the Same Name (CppCon 2021) as well. :)
I prefer the current way. This function used to be a constructor before the class' constructor was made private. This was done to prevent users from directly creating `basic_format_context` objects. I read your blog post before and I partly agree with it, however default arguments are a tool. In this case I feel the code looks better with a default argument instead of having two functions.

I agree this would be bad when the function would take several `bool` arguments with a defaulted value. IMO a function with multiple `bool` arguments often is already a code smell by itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357