[PATCH] D139277: [clangd] Use all query-driver arguments in cache key

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks a lot for taking a look at this!



================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:301
     llvm::StringRef Lang;
+    llvm::SmallVector<llvm::StringRef> AdditionalDriverArgs;
+
----------------
can we introduce a struct instead?
```
struct DriverArgs {
  std::string Driver;
  bool StandardIncludes = true;
  bool StandardCXXIncludes = true;
  bool BuiltinIncludes = true;
  llvm::StringRef Lang;
  llvm::StringRef Sysroot;

  DriverArgs(const tooling::CompileCommand &Cmd); // Traverses the Cmd and infers the bits.
  llvm::SmallVector<llvm::StringRef> render() const; // we can use canonical versions.
};
```

we can also implement hashing based on the struct now and also pass it around.


================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:304
+    // These flags will be preserved
+    const llvm::StringRef FlagsToPreserve[] = {
+        "-nostdinc", "--no-standard-includes", "-nostdinc++", "-nobuiltininc"};
----------------
i don't think we gain much by having these 3 different types now, especially considering an arg might need to be placed in multiple categories (`-isysroot` in both TwoPartArgs and ArgPrefixes).

let's rather have a check for each related bit of struct inside the for loop, e.g. something like:

```
for(...) {
  // Infer Lang
  if(Arg.startswith("-x")) {
    if (Arg == "-x" && I + 1 < E) Lang = Args[++I];
    else Lang = Arg.drop_front(2).trim();
    continue;
  }

  // Infer Sysroot
  ...
}
```


================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:310
+    const llvm::StringRef ArgPrefixesToPreserve[] = {
+        "-isysroot", "--sysroot=", "-specs=", "--specs="};
+
----------------
can we add `--specs` related changes in a follow up patch instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139277/new/

https://reviews.llvm.org/D139277