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

[Simon Marchi via Phabricator via cfe-commits]

simark 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()) {
----------------
ioeric wrote:
> 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. 
> 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.

Did it have a reason not to?  What is the `RealPathName` field useful for, if it's unreliable?


================
Comment at: lib/Basic/FileManager.cpp:326
+      llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+      UFE.RealPathName = AbsPath.str();
+    }
----------------
ioeric wrote:
> 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?
> 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?

If we don't, we probably risk having duplicate results similar to what

  https://reviews.llvm.org/D48687

fixed, by with paths differing because of symlinks instead of dot-dots.


Repository:
  rC Clang

https://reviews.llvm.org/D51159