nridge added a comment. In D138546#3946144 <https://reviews.llvm.org/D138546#3946144>, @sammccall wrote:
> In D138546#3946046 <https://reviews.llvm.org/D138546#3946046>, @cpsauer wrote: > >> Sam, my read, too, is that the memoizing design isn't safe--also that the >> key bug is preexisting. >> Nathan and I were thinking, though, that we'd should post this incremental >> fix for review rather than getting bogged down in trying to fix multiple >> things atomically. > > Sure. At first glance the design looks like it's been changed in a way that's > broken, but maybe there's some deeper reason that it's safe. That reason may > or may not also apply to `-target` (e.g. if `-target` could plausibly differ > across a project but other flags couldn't). I wanted to understand > whether/why it's broken today before concluding it's safe to break it > further. Probably @kadircet is the best person to make a call here. Given that a "project" is a fuzzy concept -- where e.g. someone in a cross-compilation scenario may choose to group host tools and target tools together in the same project -- surely `-isysroot`, `-nostdinc` etc. could also potentially differ across a project (but it's probably not common). I think the simple answer here is that not including those flags in the key was an oversight, albeit a low-impact one. I'd say taking this change as-is would be a net improvement (but we should also fix the oversight, now that we're aware of it, in a follow-up change). My only suggestion regarding this patch would be to amend this test <https://searchfox.org/llvm/rev/fc986e38d24f80c0a8525512945fe8d35b657674/clang-tools-extra/clangd/test/system-include-extractor.test#14-18> to check for the preservation of the target flag as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
