[PATCH] D130867: [clang] adds builtin `std::invoke` and `std::invoke_r`

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D130867#3759181 <https://reviews.llvm.org/D130867#3759181>, @aaron.ballman wrote:

> Thanks for working on this; these sort of improvements to compile time overhead are very much appreciated!
>
> Has this been tested against libc++ (our preccommit CI doesn't test that), and if so, do all the results come back clean from their test suite? Did you try out any other large scale C++ projects to make sure they don't break?

I've tested it with the libc++ tests (it caught an interesting thing or two), range-v3, and whatever else I've built since adding this patch to my local install of Clang. I can give a few more large projects a spin if you want higher confidence?

In D130867#3768142 <https://reviews.llvm.org/D130867#3768142>, @var-const wrote:

> Out of curiosity, would it be possible to do a benchmark to see how adding `_LIBCPP_NODEBUG` to `std::invoke` would compare to using builtins?

They're within a few hundred bytes of each other when I also apply `[[clang::always_inline]]`, but it's still worth applying the built-in. This patch helps multiple standard library implementations, so while I encourage libc++ to consider decorating `std::invoke`, I do think it's worth the implementation existing in Clang too. Another good reason is that by having it as a built-in lets me more easily add `__is[_nothrow]_invocable` and `__invoke_result`.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8411
+  "can't invoke pointer-to-%select{data member|member function}0: expected "
+  "second argument to be a %select{reference|wrapee|pointer}1 to a class "
+  "compatible with %2, got %3">;
----------------
aaron.ballman wrote:
> Something's off here, what's a "wrapee"? ("to be a wrapee to a class compatible" doesn't seem grammatically correct.)
"wrapee" refers to the type that's wrapped by `std::reference_wrapper`. I can't think of a better name for them, any suggestions?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8413-8414
+  "compatible with %2, got %3">;
+def err_invoke_pointer_to_member_drops_qualifiers : Error<
+  "can't invoke pointer-to-member function: '%0' drops '%1' qualifier%s2">;
+def err_invoke_pointer_to_member_ref_qualifiers : Error<
----------------
aaron.ballman wrote:
> Can we reuse `err_pointer_to_member_call_drops_quals` instead of making a new diagnostic?
Short answer: yes.

Long answer: I've found that "can't invoke X" to be //really// helpful when they've popped up, because it's very obvious that this is to do with `std::invoke`, so I'm partial to making `err_pointer_to_member_call_drops_quals` toggleable based on that. (I think you suggest this two comments down?)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:267
       auto *Ctor = dyn_cast<CXXConstructorDecl>(FD);
-      if (Ctor && Ctor->isInheritingConstructor())
-        Diag(Loc, diag::err_deleted_inherited_ctor_use)
-            << Ctor->getParent()
-            << Ctor->getInheritedConstructor().getConstructor()->getParent();
-      else
-        Diag(Loc, diag::err_deleted_function_use);
-      NoteDeletedFunction(FD);
+      if (!IsStdInvoke) {
+        if (Ctor && Ctor->isInheritingConstructor())
----------------
aaron.ballman wrote:
> Why do we want to suppress the diagnostic here?
I think this is the one where I was getting the same diagnostic twice.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14543
   if (Result.isNull()) {
-    S.Diag(OpLoc, diag::err_typecheck_indirection_requires_pointer)
-      << OpTy << Op->getSourceRange();
+    if (!IsStdInvoke)
+      S.Diag(OpLoc, diag::err_typecheck_indirection_requires_pointer)
----------------
aaron.ballman wrote:
> This also surprises me.
Or maybe it was this one. One (or both) of the suppressions is because there was a duplicate error.


================
Comment at: clang/test/SemaCXX/builtin-std-invoke.cpp:295
+int deleted_function() = delete; // expected-note 2 {{'deleted_function' has been explicitly marked deleted here}}
+
+struct Incompatible {};
----------------
aaron.ballman wrote:
> It would be good to have a test which decays a use of `std::invoke` into a function pointer to make sure that's still possible.
Is that allowed? I thought taking the address of `std::invoke` was ill-formed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130867