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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits