[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

Fri Nov 25 00:55:11 PST 2022

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