[libcxx-commits] [PATCH] D140911: [libc++] Implement P2505R5(Monadic operations for std::expected)

Mon Jan 16 09:29:55 PST 2023

philnik added inline comments.

================
Comment at: libcxx/include/__expected/expected.h:68
+
 namespace __expected {

----------------
yronglin wrote:
> yronglin wrote:
> > yronglin wrote:
> > > philnik wrote:
> > > > huixie90 wrote:
> > > > > philnik wrote:
> > > > > > @huixie90 Why do we have this namespace? It doesn't really seem worth it just for `__throw_bad_expected_access`.
> > > > > The honest answer is i can't remember. perhaps in some point of time I had several functions but got deleted in the end. But it seems that after this patch, there is more stuff in it.
> > > > I would remove the namespace then. It doesn't really improve the readability in any way, since every single member has `expected` already in it's name. We also don't have these kinds of detail namespaces normally.
> > > If we decided to remove __expected namespace, I think __unexpected namespace in unexpected.h also should be removed, What do you think?
> > If we decided to remove `__expected` namespace, I think `__unexpected` namespace in unexpected.h also should be removed, What do you think?
> Seems this namespace used to avoid ADL lookup?
> ```
> $ ":" "RUN: at line 15"
> note: command had no output on stdout or stderr
> $ "clang-tidy-16" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp" "--warnings-as-errors=*" "-header-filter=.*" "--checks=-*,libcpp-*" "--load=/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/libcxx/test/tools/clang_tidy_checks/libcxx-tidy.plugin" "--" "-nostdinc++" "-I" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1" "-I" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1" "-I" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/libcxx/test/support" "-std=c++2b" "-Werror" "-Wall" "-Wctad-maybe-unsupported" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" "-Wno-tautological-compare" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_ENABLE_EXPERIMENTAL" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" "-fno-modules"
> # command output:
> /home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:552:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
>       __throw_bad_expected_access<_Err>(__union_.__unex_);
>       ^
> /home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:559:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
>       __throw_bad_expected_access<_Err>(__union_.__unex_);
>       ^
> /home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:566:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
>       __throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
>       ^
> /home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:573:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
>       __throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
>       ^
> /home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:1167:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
>       __throw_bad_expected_access<_Err>(__union_.__unex_);
>       ^
> /home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:1173:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
>       __throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
>       ^
> 
> # command stderr:
> 6 warnings generated.
> Suppressed 48 warnings (48 NOLINT).
> 6 warnings treated as errors
> 
> error: command failed with exit status: 1
> 
> ```
Yes, I think we should also remove `__unexpected`, but let's do that in another patch. You have to add `std::` then calling `__throw_bad_expected_access` now to avoid the ADL lookup.

================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
+
----------------
yronglin wrote:
> philnik wrote:
> > This seems weird. Why should we ignore errors?
> This used to ignore errors like `type '_Up' (aka 'int') cannot be used prior to '::' because it has no members`, `no matching constructor for initialization of '_Up' (aka 'std::expected<int, NotSameAsInt>')`, and so on, I think we don't need to check errors like that, what do you think?
I'm not super happy with just ignoring errors, since we might miss it if they change in a relevant way. OTOH having a lot of other errors littered in between also seems like a not-so-great thing. IDK, does anybody else have thoughts here?

================
Comment at: libcxx/test/std/utilities/expected/expected.void/observers/error_or.pass.cpp:93
+  test_default_template_arg();
+  static_assert(test_default_template_arg());
+
----------------
yronglin wrote:
> philnik wrote:
> > Why is this only `static_assert`ed?
> Could you please clarify?
I thought in the diff I looked at `test_default_template_arg()` was never called at runtime. Ignore my comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911