cjdb added a comment. In D105439#2874706 <https://reviews.llvm.org/D105439#2874706>, @ldionne wrote:
> I'm not entirely sure I understand the purpose of this patch. So the idea is > that let's say a tool suggests including `<__algorithm/find.h>` to get the > definition of `std::find` as a IWYU fix-it sort of suggestion, the user would > naively do that, and then the compiler (with this patch) would error out > saying "woops, you can't include that libc++ detail header". Is that the idea? > > If that's it, then I would much rather fix the tools that incorrectly suggest > including those implementation detail headers in the first place. ... I think > it's great to try and give the proper diagnostic to users, but I think the > correct place to do that is in the tool that suggests it in the first place. > Thoughts? There's way more reviewer focus on tooling in this patch than there should be. Fixing non-LLVM tools is not a feasible request: I barely have enough time to try and make sure that LLVM tooling is going to respond to our libc++ changes. The point of this patch is to help //users// in a variety of situations. 1. They explicitly include a detail libc++ header. I've observed this quite frequently with the `bits/` headers in libstdc++, and even Boost, and while it might seem like a good idea at the time, it's going to inevitably cause pain down the road (as I have experienced when trying to upgrade an employer's Boost library). This is akin to wearing a seatbelt. 2. Tools that auto-include the detail thing and then get checked into a library's trunk without the author noticing (this happens to me more often than I'd care to admit). This can impact downstream users. I don't really have a good car analogy for this situation, but it lowers the chance for negligence. 1. Yes, tool QoI is important and should be fixed too, but //people// have a habit of trusting their tools, and this is a good way to get tools to improve their QoI. 2. Putting the burden on tooling also doesn't take into consideration that some people are neurodiverse, and it might be difficult for them to even remember to go back and check that the "IWYU fix-it" is correct. That's not complacency or naivety or inexperience; this is how some people's brains are hardwired. 3. Hyrum's law is boss here. It doesn't matter how much we improve external tooling: if a user //can// include our details, some folks inevitably will. > Users will be less confused and we won't have to special-case a special > directory name, which I can imagine could cause issues. I'm not sure why users will be more confused if we do this? ----- I do plan to improve QoI for LLVM tooling in other areas, but this patch is essentially a catch-all handbrake. It's best if you put attention on people, and human behaviour: this is a human problem that I'm trying to solve, not a technical one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105439/new/ https://reviews.llvm.org/D105439 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
