[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

This revision was automatically updated to reflect the committed changes. Closed by commit rG3ee277b86b34: [Support] add vfs support for ExpandResponseFiles (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. In D70769#1763703 , @kadircet wrote: > @jhenderson I am planning to commit this if the discussion around `std::errc` > vs `llvm::errc` is resolved, I don't have any preference towards one or the > other both seems to get the

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

kadircet added a comment. @jhenderson I am planning to commit this if the discussion around `std::errc` vs `llvm::errc` is resolved, I don't have any preference towards one or the other both seems to get the work done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231510. lh123 marked an inline comment as done. lh123 added a comment. fix typo: "Could not convert UTF16 To UTF8" -> "Could not convert UTF16 to UTF8" Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 marked 3 inline comments as done. lh123 added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8");

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8"); lh123 wrote: > jhenderson

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 marked an inline comment as done. lh123 added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8");

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8"); jhenderson wrote: >

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8");

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231390. 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/D70769/new/ https://reviews.llvm.org/D70769 Files:

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1071 + return llvm::make_error( + "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode()); Str = StringRef(UTF8Buf); Not sure, but you might want to use

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, please make sure you've clang-formatted the changes though Comment at: llvm/lib/Support/CommandLine.cpp:1054 + llvm::ErrorOr CurrDirOrErr =

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231384. lh123 added a comment. fixes no matching constructor for initialization of `llvm::StringError`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files:

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231381. lh123 added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: llvm/include/llvm/Support/CommandLine.h

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231377. lh123 added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: llvm/include/llvm/Support/CommandLine.h

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1148 + llvm::ErrorOr RHS = FS.status(RFile.File); + if (!LHS || !RHS) { +return false; kadircet wrote: > again you need to `consumeError`s before returning. I would

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231361. lh123 added a comment. Sorry, upload the wrong patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: llvm/include/llvm/Support/CommandLine.h

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231359. Herald added a subscriber: mgorny. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: clang/include/clang/Tooling/CompilationDatabase.h

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231353. lh123 added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: llvm/include/llvm/Support/CommandLine.h

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

kadircet added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1053 + llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory(); + if (!CurrDir) +return false; this comment has moved into a irrelevant line and wasn't addressed, so re-stating

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

merge_guards_bot added a comment. Build result: pass - 60338 tests passed, 0 failed and 732 were skipped. Log files: console-log.txt , CMakeCache.txt

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 marked 3 inline comments as done. lh123 added inline comments. Comment at: llvm/include/llvm/Support/CommandLine.h:47 +class FileSystem; +IntrusiveRefCntPtr getRealFileSystem(); + kadircet wrote: > now that we are also pulling the the function, I think it

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

kadircet added inline comments. Comment at: llvm/include/llvm/Support/CommandLine.h:47 +class FileSystem; +IntrusiveRefCntPtr getRealFileSystem(); + now that we are also pulling the the function, I think it is OK to include the header instead.

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. Aside from one nit, this looks okay to me. I'd like someone else to look at it though too. I'm not at all familiar with the file system stuff to be confident to say everything there is good. Comment at: llvm/lib/Support/CommandLine.cpp:1148 +

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231234. lh123 added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: llvm/include/llvm/Support/CommandLine.h

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

merge_guards_bot added a comment. Build result: fail - 60341 tests passed, 3 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 failed:

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231215. lh123 added a comment. address comments and delete unused code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: clang/tools/driver/driver.cpp lld/COFF/DriverUtils.cpp

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 updated this revision to Diff 231213. lh123 added a comment. Address comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 Files: clang/tools/driver/driver.cpp lld/COFF/DriverUtils.cpp lld/ELF/DriverUtils.cpp

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. In D70769#1761517 , @sammccall wrote: > It's not just unit-tests, in D70222 we will > ultimately use FSes other than getRealFilesystem() in clangd. I see, thank you. > Having non-VFS-clean

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

sammccall added a comment. In D70769#1761494 , @jhenderson wrote: > In D70769#1761428 , @lh123 wrote: > > > In D70769#1761394 , @jhenderson > > wrote: > > > > > Are there

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. In D70769#1761428 , @lh123 wrote: > In D70769#1761394 , @jhenderson > wrote: > > > Are there any instances where we DON'T want to get the real file system? If > > not, could the

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 added a comment. In D70769#1761394 , @jhenderson wrote: > Are there any instances where we DON'T want to get the real file system? If > not, could the `*llvm::vfs::getRealFileSystem()` call be put inside > `cl::ExpandResponseFiles`? This patch

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. Are there any instances where we DON'T want to get the real file system? If not, could the `*llvm::vfs::getRealFileSystem()` call be put inside `cl::ExpandResponseFiles`? Comment at: llvm/include/llvm/Support/CommandLine.h:32 #include

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

merge_guards_bot added a comment. Build result: pass - 60330 tests passed, 0 failed and 732 were skipped. Log files: console-log.txt , CMakeCache.txt

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

lh123 created this revision. lh123 added a reviewer: kadircet. Herald added subscribers: llvm-commits, cfe-commits, seiya, rupprecht, MaskRay, jakehehrlich, aheejin, hiraditya, arichardson, sbc100, emaste. Herald added a reviewer: espindola. Herald added a reviewer: alexshap. Herald added a