[libcxx-commits] [PATCH] D150060: [libc++][format] Removes optional dependency.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__format/format_context.h:17
 #include <__format/buffer.h>
 #include <__format/format_arg.h>
 #include <__format/format_arg_store.h>
----------------
IMO it would be nice to be able to keep using `optional` here. I am not concerned about potential circular dependency problems because we could always granularize `<optional>` into `<__optional/optional.h>` and `<__optional/formatter.h>`. Then here you'd include `<__optional/optional.h>` only.


================
Comment at: libcxx/include/__format/format_context.h:71-75
+  _LIBCPP_HIDE_FROM_ABI __locale(__locale&& __other) noexcept : __dummy_{'\0'}, __engaged_(__other.__engaged_) {
+    // Note locale is not movable.
+    if (__engaged_)
+      __loc_ = __other.__loc_;
+  }
----------------
It's surprising to provide a move constructor here if the underlying locale is not movable. Also, should we be disengaging `__other`?


================
Comment at: libcxx/include/__format/format_context.h:77-85
+  _LIBCPP_HIDE_FROM_ABI __locale& operator=(const __locale& __other) noexcept {
+    __assign(__other);
+    return *this;
+  }
+
+  _LIBCPP_HIDE_FROM_ABI __locale& operator=(__locale&& __other) noexcept {
+    __assign(__other);
----------------
If you retain this, you could inline `__assign` into `operator=(__locale const&)` and call `operator=(__locale const&)` from `operator=(__locale&&)`. And actually you don't need to provide `operator=(__locale&&)` at all, since the copy-assignment will be used if you do `x = std::move(y)` and `x` only has a copy-assignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150060