[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