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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: libc++.
aaron.ballman added a comment.

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?



================
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">;
----------------
Something's off here, what's a "wrapee"? ("to be a wrapee to a class compatible" doesn't seem grammatically correct.)


================
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<
----------------
Can we reuse `err_pointer_to_member_call_drops_quals` instead of making a new diagnostic?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8415
+  "can't invoke pointer-to-member function: '%0' drops '%1' qualifier%s2">;
+def err_invoke_pointer_to_member_ref_qualifiers : Error<
+  "can't invoke pointer-to-member function: '%0' can only be called on an "
----------------
Same question here, but about `err_pointer_to_member_oper_value_classify` instead.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8418-8430
+def err_invoke_wrong_number_of_args : Error<
+  "can't invoke %select{function|block|pointer-to-member function}0: expected "
+  "%1 %select{argument|arguments}2, got %3">;
+def err_invoke_function_object : Error<
+  "can't invoke %0 function object: %select{no|%2}1 suitable "
+  "overload%s2 found%select{|, which makes choosing ambiguous}1">;
+def err_invoke_function_object_deleted : Error<
----------------
Can we modify existing diagnostics for these as well?

It seems like in most cases, it's a matter of adding a select to use `invoke` terminology in all of these cases, which is why I'm asking.


================
Comment at: clang/include/clang/Sema/Sema.h:4099
                                      UnaryOperatorKind Opc,
-                                     const UnresolvedSetImpl &Fns,
-                                     Expr *input, bool RequiresADL = true);
+                                     const UnresolvedSetImpl &Fns, Expr *input,
+                                     bool RequiresADL = true,
----------------
Since we're already touching the line anyway...


================
Comment at: clang/include/clang/Sema/Sema.h:4101
+                                     bool RequiresADL = true,
+                                     bool IsStdInvoke = false);
 
----------------
Hmmm, I wonder if there's a way we can make this change without having to force every caller to think about `std::invoke` when they look at the signature for these calls? For example (and maybe this is a terrible idea, I'm thinking out loud), could we use an RAII object that specifies we're in a "building a call to invoke" context that is then checked within these functions (and we leave that RAII object before evaluating arguments to the invoke)?

I'm not strongly opposed to the current approach, but I'm worried that it adds complexity for an infrequent construct. I think we should try to keep the situations where we need information about a specific API as self-contained as possible because WG21 has shown more and more interest in the library being implemented in the compiler (so there's a scaling concern as well).


================
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())
----------------
Why do we want to suppress the diagnostic here?


================
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)
----------------
This also surprises me.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5713-5715
+  if (B.isInvalid()) {
+    return ExprError();
+  }
----------------
Down with braces! Up with danger!


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5736-5739
+  auto *Fn = CalleeType->isMemberFunctionPointer()
+                 ? HandleInvokePointerToMemberFunction
+                 : HandleInvokePointerToDataMember;
+  return Fn(S, CalleeType, IsInvokeR, LParenLoc, F, Args, RParenLoc);
----------------
Yeah, it repeats a bunch of arguments to the calls, but at least it's not using function pointers and hoping the optimizer will undo them back into direct calls.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5759
+                                              SourceLocation RParenLoc) {
+  auto *PtrToMember = CalleeType->getAs<MemberPointerType>();
+  if (Args.size() == 0 ||
----------------
Adding some standards citations about why you're handling things the way you are would be appreciated.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5776-5777
+  if (RecordDecl *D = FirstArgType->getAsCXXRecordDecl()) {
+    bool IsReferenceWrapper =
+        D->isInStdNamespace() && D->getName() == "reference_wrapper";
+    if (IsReferenceWrapper) {
----------------
Do we need to look at the canonical type of `FirstArgType`? e.g., what if the user did something odd like wrapped reference_wrapper in a type alias and then used that?


================
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 {};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130867