aaron.ballman accepted this revision. aaron.ballman added a comment. In D121328#3406017 <https://reviews.llvm.org/D121328#3406017>, @dblaikie wrote:
> @aaron.ballman wouldn't mind your take on this to see if this seems > sufficiently robust, tested, etc. (should I move the isExternallyVisible > check even further down? So its side effects are even less impactful (maybe > there are other warnings that care about this sort of thing?) ) Oh this turned out to be a MUCH easier solution than I was thinking, good catch! The changes LGTM as-is. I think we could potentially move this check further down given that we know it has side effects. I *think* it'd be fine directly above the `for` loop, at least from looking at the implementations of those other functions, I don't see others that are caching bits in the way that `isExternallyVisible()` ends up doing. However, I don't insist on moving it, either. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121328/new/ https://reviews.llvm.org/D121328 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
