ldionne wrote:
> > AFAICT we have one test for `invoke` plus a constexpr test which should > really just be rolled into the generic `invoke` test. That test is ~400 LoC, > which is definitely not nothing, but also not unmanageable IMO. We could > probably also simplify that by not explicitly listing every cv combination > but instead using `types::for_each`. As long as libc++ keeps testing its public API fully, I'm OK with this. What I don't want is to start shifting test burden towards Clang because the compiler actually implements most of the functionality. These builtins should be implementation details, so we should still be testing the full public API in libc++ itself. The testing question from the Clang side is interesting and I would expect Clang folks to have an opinion. From the Clang side, I'd expect some concerns about adding a builtin with such a complex and user-visible "API". But those concerns are not really for me to raise, if Clang folks are fine with it, that's not an issue for me. > > Yes, it does. If that is the main point of contention we can move that into a > separate patch. I don't expect the compile times to differ significantly > between recognizing `std::invoke` as a builtin itself and having > `std::invoke` use `__builtin_invoke` as the implementation. It would still be > nice for improved debug performance and reduced debug info though. > > What exactly is the difference you perceive between the two? I mean, sure, > there is one, but if `std::invoke` just always calls `__builtin_invoke` there > really isn't a meaningful one. > > [...] > > Is your main concern here that we'd be limited in the library what we can > change without the changing the compiler? If that is the case, would your > concerns be alleviated if we could opt out of the builtin recognition for > specific overloads, e.g. through an attribute? As discussed just now in the libc++ monthly: - Silently hijacking a libc++ function by the compiler is surprising and certainly unexpected unless you already know what's going on. I can imagine someone trying to fix a bug in `std::invoke` and spending hours scratching their heads not understanding why `std::invoke` is still not doing the right thing after their changes, only to find out that the compiler was really implementing the function under the hood. These things should be explicit. - Silently hijacking also means that we lose control over what's happening from the library side, so a bug or an improvement requires changes to the compiler. My view is that compiler builtins should be tools to implement an API more simply and more efficiently. They shouldn't *be* the API itself. Personally, I think the best way to move this forward would be to remove the hijacking from this patch to make it less contentious, and then to potentially pursue an attribute to formalize how hijacking works. This could (and should) be used beyond `std::invoke`, for example with `std::forward` and `std::move`. That would actually be an improvement to the status-quo in relation to my concerns with "discoverability". https://github.com/llvm/llvm-project/pull/116709 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
