bnbarham added inline comments.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1922
+ EC = FS->makeAbsolute(FullPath, Name);
+ Name = canonicalize(Name);
+ } else {
----------------
Why the canonicalization?
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1933-1935
path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
? sys::path::Style::posix
: sys::path::Style::windows_backslash;
----------------
`windows_slash` is going to be `windows_backslash` here. I assume this was fine
since `sys::fs::make_absolute(Name)` would always return the native style, but
that isn't the case now. It's also unfortunate we calculate this again when
`makeAbsolute` only just did it.
================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+ // with native path seperator, regardless of the actual path seperator
+ // used in YAMLFilePath field.
+#ifndef _WIN32
----------------
`separator` -> `separator`
As per my comment above, this is only true because we're either using posix or
windows_backslash. I'm not sure what we really want here, should we always
convert to native? Any thoughts @dexonsmith?
FWIW the non-windows case is identical to the above block. It'd be interesting
to test `/` and `\` on both filesystems.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137473/new/
https://reviews.llvm.org/D137473
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
