[PATCH] D121733: Clean pathnames in FileManager.
Ben Barham via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 18 11:13:57 PDT 2022
bnbarham added a comment.
In D121733#3392931 <https://reviews.llvm.org/D121733#3392931>, @dexonsmith wrote:
> However, FileManager changes sometimes have odd side effects... and it's possible that somewhere in clang relies on having `FileManager::getFileRef()` return precisely the same path that was requested. Tagging a few other people that have some context... please share your opinions!
Wouldn't that already not be the case though (ie. given `RedirectingFileSystem` and `use-external-names` is currently a thing)? I know we're wanting to change that, but I don't *know* of anywhere that depends on this currently.
> @ppluzhnikov, can you give more context on how this interacts with https://reviews.llvm.org/D121658? I had a quick look but it wasn't immediately obvious.
If I understand correctly, the failing tests in that patch are failing because they're always expecting "/" and since `sys::append` is used, it's now "\\" on Windows. The remove dots change doesn't fix those, since the tests would still need to be updated to remove the "./" (which is most of the tests in this patch). But there's also some others where I wouldn't expect them to be failing in this patch, eg. the ones from `/` -> `{{[/\\]}}`.
Are there tests that we can't just fix to expect either `/` or `\\`? Why do we need to change the underlying behaviour here at all?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121733/new/
https://reviews.llvm.org/D121733
More information about the cfe-commits
mailing list