[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: clang/include/clang/Basic/FileManager.h:110
+/// A reference to a \c FileEntry that includes the name of the file as it was
+/// accessed by the FileManager's client.
+class FileEntryRef {
----------------
bruno wrote:
> How does that work with the VFS? Will it keep the virtual path as the FileName to return with getName() or the external-content? Should it change when in face of `use-external-names` attribute?
Good catch. In the first patch `getFileRef` didn't honor `use-external-names` correctly (specifically once a file was opened once, for subsequent open of that file, it returned the VFS name, not the external name). 

I fixed this issue by introducing a potential indirection to `SeenFileEntries` in the update patch. Now the `getFileRef` should always return the virtual path, unless `use-external-names` is specified, and then it will return the external path.


================
Comment at: clang/include/clang/Basic/FileManager.h:130
+
+  const DirectoryEntry *getDir() const { return Entry.getDir(); }
+
----------------
Bigcheese wrote:
> Isn't this incorrect in the case of symlinks?
Right, I was planning to provide another way to get a directory with name. For now I'll just remove this method, so clients will have to call into FileEntry.


================
Comment at: clang/include/clang/Basic/FileManager.h:249-251
+  /// This function is deprecated and will be removed at some point in the
+  /// future, new clients should use
+  ///  \c getFileRef.
----------------
bruno wrote:
> Bigcheese wrote:
> > `LLVM_DEPRECATED()`?  (or w/e the name is of our depreciation attribute macro).
> Probably can't be done until we move all uses over? There's probably still enough of them that the warning would be very annoying?
That's right, I can't mark the function as deprecated just yet, as you'll get a lot of warnings when building clang.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65907