AaronBallman wrote: > > > After it's done, the builtin provided here can be either removed or > > > replaced with a version wrapping the more efficient one > > > > > > Apologies if I've missed something in the various conversations, but this > > catches my eye as a concern. Once we release the builtin to the wild, > > people will start using it, which means removing or replacing it becomes > > more effort for us and more pain for our users. So I'm worried we'll end up > > in the very bad situation where we have two builtins that do very similar > > things and users have to figure out which to use and why. > > I understand the worry, but I don't think it's that severe. If we explicitly > tell people the builtin may be removed in the future versions and ask them to > ensure they put `__has_builtin` around it, we can ensure they code keeps > working.
We can hope, but we know those hopes don't always pan out how we expect, too. :-) > Moreover, if we ever remove it, we will ship an alternative in the same > release. That requires a refactoring, sure, but it looks very manageable > (e.g. people can slap a wrapper I've shown above in their codebase and > search-and-replace the builtin with their version). Yeah, it's the "if" in "if we ever remove it" that has me worried. If we *don't* remove it, users now have to figure out what subtle differences exist between the APIs. And if it's deprecated, they still have to figure those out. > If this is not convincing, would adding a `-cc1` flag and making the builtin > available only when it's enabled in the meantime be something that you are > open to? I had this as a backup plan all along, and I am willing to commit to > doing any follow-up work to leave no trace of it after #106730 lands. As I've > mentioned before, having this builtin as early as possible would really help > us, and we're willing to take on some complexity and do extra work to land it > earlier. In some regards that helps, in other regards it expands the problem because now we may end up with folks relying on the -cc1 argument existing. > > Is there something holding up #106730? It'd be better for us to land the > > correct solution instead of a stopgap that may end up having unfortunate > > extra costs associated with it? > > The implementation complexity is much higher there and it's also still > incomplete (look at TODOs in the description and also for FIXMEs in the added > test) and I expect a certain level of discussions and changes to be required > there after the feedback comes in based on previous iterations. Introducing a > new language construct for a builtin that could be implemented using existing > C++ is quite a bit of a scope creep to meet the actual goal that I had in the > start (improved compile times for a targetted template metaprogramming > pattern). I am still wiling to make it happen and continue down that path, > hopefully this helps others, but I'd really love to get the initial outcomes > ASAP. It sounds like the other review may not be as far away from landing as originally thought: https://github.com/llvm/llvm-project/pull/106730#issuecomment-2876979768 Because this isn't for correctness but is for performance, I think there isn't a pressing need to land something ASAP from the community perspective (or is the performance truly bad enough that you think the feature is not really usable without doing something?). So could you keep the temporary changes in your downstream until the full changes are ready upstream? (I'm pushing back because we have a historical pattern of temporary solutions becoming permanent solutions. If the solution didn't involve introducing new user-facing functionality, I'd be less concerned but because it is user-facing, the concerns are higher. Imagine a world where GCC or EDG then adds the same [temporary] builtin for Clang compatibility, for an example of another kind of problem we have to worry about.) https://github.com/llvm/llvm-project/pull/139730 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
