aaron.ballman added a comment. The code changes look good to me, the only real question I have left is whether this will be enabled by enough folks to be worth adding a default-off diagnostic. My intuition is that this is useful functionality for the folks who use `-icf=all` and when I look around to see if that option is being used, I see a terrifying number of uses of it given how broken I consider its semantics to be: https://sourcegraph.com/search?q=context:global+-icf%3Dall+-file:.*test.*&patternType=standard&sm=1&groupBy=repo
My thinking is that it's worthwhile to add this even though it's off-by-default, but hopefully we can find ways to encourage its use better. For example. it might be nice to suggest users enable this warning from the linker documentation about the `icf` option, or a blog post on the warning, etc. But I don't think that's a requirement to accept this. That said, I'm also curious about the details of how well users are able to react to this diagnostic (what's the false positive rate, can users make the corrections they need from the diagnostic, etc), so I'd prefer to hold off on landing this for a little bit until we have some idea of that from the chromium folks. (Given the branch is happening next week, I think this should wait to land until after the branch anyway so it has more time to bake.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141310/new/ https://reviews.llvm.org/D141310 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
