[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb abandoned this revision. jfb added a comment. Let's do the better fix that Michael suggested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 ___

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214477. jfb added a comment. - Also update function name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driver.h

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214448. jfb marked an inline comment as done. jfb added a comment. - Rename option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files:

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/include/clang/Driver/Options.td:2012 + HelpText<"Use the virtual file system in 'real' mode, or 'physical' mode. In 'real' mode the working directory is linked to the process' working directory.

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere added inline comments. Comment at: clang/include/clang/Driver/Options.td:2012 + HelpText<"Use the virtual file system in 'real' mode, or 'physical' mode. In 'real' mode the working directory is linked to the process' working directory. In 'physical' mode it has

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added a comment. In D65986#1623283 , @Bigcheese wrote: > This fix works, but we could also use openat to get around max path length > issues. Windows also has an API that can be used similarly. Given the type

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

Bigcheese added a comment. This fix works, but we could also use openat to get around max path length issues. Windows also has an API that can be used similarly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang/test/Driver/vfsmode.py:47 +# exceed PATH_MAX. +assert os.path.exists(file_relative_path) +assert not os.path.exists(os.path.join(absolute, filename)) bruno wrote: > If you want to

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214397. jfb added a comment. - Document asserts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driver.h

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

bruno accepted this revision. bruno added a comment. LGTM Comment at: clang/test/Driver/vfsmode.py:47 +# exceed PATH_MAX. +assert os.path.exists(file_relative_path) +assert not os.path.exists(os.path.join(absolute, filename)) If you want to make this even more

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked 4 inline comments as done. jfb added inline comments. Comment at: clang/include/clang/Driver/Options.td:2010 HelpText<"Overlay the virtual filesystem described by file over the real file system">; +def ivfsmode : Joined<["-"], "ivfsmode=">, Group,

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added a comment. In D65986#1622447 , @sammccall wrote: > Tagging D62271 and @Bigcheese as this was > the change that switched the default in Driver. (My motivation for D58169 >

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang/test/Driver/vfsmode.py:4 + +# UNSUPPORTED: system-windows + labath wrote: > jfb wrote: > > I'm not sure what the best way to test this on Windows would be, and > > without a

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

sammccall added a subscriber: Bigcheese. sammccall added a comment. Tagging D62271 and @Bigcheese as this was the change that switched the default in Driver. (My motivation for D58169 was thread-heavy programs like clangd).

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

labath added inline comments. Comment at: clang/test/Driver/vfsmode.py:4 + +# UNSUPPORTED: system-windows + jfb wrote: > I'm not sure what the best way to test this on Windows would be, and without > a machine handy I can't really test this :-( Unsupported

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere accepted this revision. JDevlieghere added a comment. Thanks for bearing with me, JF! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 ___

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; JDevlieghere wrote: > jfb wrote: > > JDevlieghere wrote: > > > Is this really

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214298. jfb marked an inline comment as done. jfb added a comment. - Address more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files:

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; jfb wrote: > JDevlieghere wrote: > > Is this really necessary? > the `Driver` ctor takes a `VFS` parameter

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214296. jfb marked an inline comment as done. jfb added a comment. - Address more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files:

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Two small suggestions for the test, but this change LGTM. Comment at: clang/test/Driver/vfsmode.py:33 +new_name_len = target_len - len(absolute) - 1 +

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214292. jfb added a comment. - Undo whitespaces change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driver.h

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; JDevlieghere wrote: > Is this really necessary? the `Driver` ctor takes a `VFS` parameter which some tools set

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214291. jfb marked 6 inline comments as done. jfb added a comment. - Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files:

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; Is this really necessary? Comment at: clang/lib/Driver/Driver.cpp:980 + // We might

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

arphaman added inline comments. Comment at: clang/lib/Driver/Driver.cpp:167 +else + Diag(diag::err_drv_unsupported_option_argument) << OptionPrefix << Value; + } It might be nice to `break` here to exit early, unless you want to make Clang pick the

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/test/Driver/vfsmode.py:4 + +# UNSUPPORTED: system-windows + I'm not sure what the best way to test this on Windows would be, and without a machine handy I can't really test this

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb created this revision. jfb added reviewers: JDevlieghere, bruno. Herald added subscribers: cfe-commits, dexonsmith, jkorous. Herald added a project: clang. The motivation for 'physical' mode in D58169 was pretty sensible, but it has one unfortunate