[PATCH] D69958: Improve VFS compatibility on Windows

Wed Nov 13 16:04:19 PST 2019

amccarth marked an inline comment as done.
amccarth added a comment.

Thanks for the review.

In D69958#1744861 <https://reviews.llvm.org/D69958#1744861>, @vsapsai wrote:

> There is one issue with a comment, apart of that everything looks good to me. Looked into replacing slash and backslash literals with calls to `llvm::sys::path::get_separator` or `llvm::sys::path::is_separator` but in my attempts failed to improve existing patch.

Yes, I originally started down the path of trying to "normalize" path styles (like slash direction), but that always led to collateral damage because we literally have situations where a path is a hybrid of both Windows and Posix.  Rather than writing Windows and Posix versions of the tests, this patch takes the approach that choosing a path style is out-of-scope, which I'm told is Clang's general strategy.

================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:535
 /// All configuration options are optional.
 ///   'case-sensitive': <boolean, default=true>
 ///   'use-external-names': <boolean, default=true>
----------------
vsapsai wrote:
> I believe changing default `CaseSensitive` value makes this comment incorrect. Have no strong opinion on making case-insensitive default on Windows because a better requirement is for VFS generators to specify 'case-sensitive' explicitly instead of relying on default values.
Make CaseSensitive system-dependent aligns with UseCanonicalizePaths, so this feels pretty consistent.  (Perhaps in a future path, we can fix both of those.)  In the mean time, I'll update the comment.

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

https://reviews.llvm.org/D69958