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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
