[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-    UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+    if (auto Path = UFE.File->getName()) {
----------------
simark wrote:
> ioeric wrote:
> > simark wrote:
> > > What's the rationale for only computing the field if `UFE.File` is non-null?
> > > 
> > > Previously, if you looked up the file with `openFile == false` and then later `openFile == true`, the `RealPathName` field would not be set because of this.  That doesn't seem right.
> > There has been no guarantee that RealFilePath is always set. I think that's the reason why the acceasor is called tryGetRealPathName.
> The way I understood it was that it could be empty because computing the real path can fail.  Not just because we didn't skipped computing it.
I agree that the API is confusing and lack of documentation (and we should fix), but the previous implementation did skip the computation if File is not available though. 


================
Comment at: lib/Basic/FileManager.cpp:326
+      llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+      UFE.RealPathName = AbsPath.str();
+    }
----------------
simark wrote:
> ioeric wrote:
> > simark wrote:
> > > If the path contains symlinks, doesn't this put a non-real path in the RealPathName field?  Won't users (e.g. clangd) use this value thinking it is a real path, when it is actually not?
> > This was the original behavior. In general, File Manager should never call real_path for users because it can be very expensive. Users should call real_path if they want to resolve symlinks. That said, it's fair to say that "RealPathName" is just a wrong name, and we should clean it up at some point.
> Ok, then if the goal is not to actually have a real path (in the realpath(3) sense), that's fine.  But I think we should rename the field sooner than later, it's really confusing.
> 
> That also means that it's kind of useless for us in clangd, so we should always call real_path there and not rely on that field.
> Ok, then if the goal is not to actually have a real path (in the realpath(3) sense), that's fine. But I think we should rename the field sooner than later, it's really confusing.
+1

> That also means that it's kind of useless for us in clangd, so we should always call real_path there and not rely on that field.
I guess it depends on whether you want symlink resolution or not. I know that clangd's index symbol collector resolves symlink explicitly, but I don't think clangd enforces symlink resolution in general?


Repository:
  rC Clang

https://reviews.llvm.org/D51159