[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

ppluzhnikov added inline comments.


================
Comment at: clang/lib/Basic/FileManager.cpp:218
+  llvm::sys::path::remove_dots(CleanFilename, /*remove_dot_dot=*/false);
+  Filename = CleanFilename;
+
----------------
kadircet wrote:
> this is actually breaking the [contract of FileEntryRef](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileEntry.h#L58-L59), is this the point?
> 
> ```
> /// A reference to a \c FileEntry that includes the name of the file as it was
> /// accessed by the FileManager's client.
> ```
> 
> I don't see mention of this contract being changed explicitly anywhere, if so can you mention it in the commit message and also update the documentation? (the same applies to DirectoryEntryRef changes as well)
> 
> I am not able to take a detailed look at this at the moment, but this doesn't feel like a safe change for all clients. Because people might be relying on this contract without explicitly testing the behaviour for "./" in the filenames. So tests passing (especially when you're just updating them to not have `./`) might not be implying it's safe.
I chased this comment down to commit 4dc5573acc0d2e7c59d8bac2543eb25cb4b32984.

The commit message says:

   This commit introduces a parallel API to FileManager's getFile: getFileEntryRef, which returns
    a reference to the FileEntry, and the name that was used to access the file. In the case of
    a VFS with 'use-external-names', the FileEntyRef contains the external name of the file,
    not the filename that was used to access it.

So it appears that the comment itself is not quite correct.

It's also a bit ambiguous (I think) -- "name used to access the file"
could be interpreted as the name which clang itself used to access
the file, and not necessarily the name client used.

> people might be relying on this contract without explicitly testing the behaviour for "./" in the filenames.

That's a possibility.

I am not sure how to evaluate the benefit of this patch (avoiding noise everywhere) vs. possibly breaking clients.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121733