[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 231393. lh123 marked 5 inline comments as done. lh123 added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added a comment. mostly LG, a few last nits. Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:51 + // FIXME: we should rather propagate the current directory into + // ExpandResponseFiles as well in addition to FS and someone can take

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

merge_guards_bot added a comment. Build result: FAILURE - Log files: console-log.txt , CMakeCache.txt Repository: rG

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 231357. lh123 marked 6 inline comments as done. lh123 added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added inline comments. Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:28 + llvm::IntrusiveRefCntPtr FS) + : Base(std::move(Base)), Tokenizer(Tokenizer), FS(FS) { +assert(this->Base != nullptr); nit:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

merge_guards_bot added a comment. Build result: FAILURE - Log files: console-log.txt , CMakeCache.txt Repository: rG

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 231235. lh123 added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/include/clang/Tooling/CompilationDatabase.h

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

merge_guards_bot added a comment. Build result: fail - 60344 tests passed, 2 failed and 732 were skipped. failed: Clang.CodeGen/catch-implicit-integer-sign-changes-incdec.c failed: Clang.CodeGen/catch-implicit-signed-integer-truncations-incdec.c Log files: console-log.txt

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 231217. lh123 set the repository for this revision to rG LLVM Github Monorepo. lh123 added a comment. rebase to D70769 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added inline comments. Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167 +for (auto : Cmds) { + expandResponseFiles(Cmd, Tokenizer); +} lh123 wrote: > lh123 wrote: > > lh123 wrote: > > > kadircet wrote: > > > >

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 marked an inline comment as done. lh123 added inline comments. Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167 +for (auto : Cmds) { + expandResponseFiles(Cmd, Tokenizer); +} lh123 wrote: > lh123 wrote: > >

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 marked an inline comment as done. lh123 added inline comments. Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167 +for (auto : Cmds) { + expandResponseFiles(Cmd, Tokenizer); +} lh123 wrote: > kadircet wrote: > > so

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 231051. lh123 added a comment. address comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/include/clang/Tooling/CompilationDatabase.h clang/lib/Tooling/CMakeLists.txt

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 marked an inline comment as done. lh123 added inline comments. Comment at: clang/lib/Tooling/CompilationDatabase.cpp:402 llvm::sys::path::append(DatabasePath, "compile_flags.txt"); -return FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage); +

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 marked an inline comment as done. lh123 added inline comments. Comment at: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp:167 +for (auto : Cmds) { + expandResponseFiles(Cmd, Tokenizer); +} kadircet wrote: > so it looks like we

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added inline comments. Comment at: clang/include/clang/Tooling/CompilationDatabase.h:223 +/// Returns a wrapped CompilationDatabase that will expand all response files on +/// commandline returned by underlying database. nit: `s/response

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 230756. lh123 added a comment. Expand response files before `inferMissingCompileCommands` and `inferTargetAndDriverMode` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 230752. lh123 added a comment. Herald added a subscriber: mgorny. Address comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/include/clang/Tooling/CompilationDatabase.h

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added a comment. Sorry wrote comments but forgot to hit submit :( Comment at: clang/include/clang/Tooling/CompilationDatabase.h:229 +/// \return true if all @files were expanded successfully or there were none. +bool expandResponseFiles(tooling::CompileCommand , +

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 230060. lh123 added a comment. fixes some bug and add more test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/include/clang/Tooling/CompilationDatabase.h

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 230033. lh123 added a comment. fix typo in function document. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/include/clang/Tooling/CompilationDatabase.h

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 230031. lh123 edited the summary of this revision. lh123 added a comment. Address comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/include/clang/Tooling/CompilationDatabase.h

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added a comment. Thanks, layering seems better now. But as Sam mentioned in the issue it would be better to have something like `expandRSPFiles` similar to `inferMissingCompileCommands`. As the problem is not in the `JSONCompilationDatabase` itself, it can also occur in

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 229812. lh123 added a comment. Respect `JSONCommandLineSyntax` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/lib/Tooling/JSONCompilationDatabase.cpp Index:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 229807. lh123 added a comment. Move the implementation to `JSONCompilationDatabase`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang/lib/Tooling/JSONCompilationDatabase.cpp Index:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 added a comment. In D70222#1749533 , @kadircet wrote: > Thanks for taking a look into this, the `rsp files` issue has came up before > in the past but there wasn't enough investment to implement it. > > Haven't checked the implementation in detail

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

kadircet added a comment. Thanks for taking a look into this, the `rsp files` issue has came up before in the past but there wasn't enough investment to implement it. Haven't checked the implementation in detail yet, I believe the layering should be different; This is a common problem for all

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 229759. lh123 added a comment. format patch CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp Index:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 updated this revision to Diff 229758. lh123 added a comment. update diff CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 Files: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp Index:

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 added a comment. I don't know much about how to write unit tests, but it works fine on my computer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70222/new/ https://reviews.llvm.org/D70222 ___

[PATCH] D70222: [clangd] Add support for .rsp files in compile_commands.json

lh123 created this revision. lh123 added reviewers: sammccall, ilya-biryukov, hokein. lh123 added a project: clang-tools-extra. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Add support for .rsp files in