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