[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()) {
----------------
ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > 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?
> > > I think the biggest concern is the performance.
> > >  
> > > For example, clangd code completion latency increased dramatically with `real_path`:
> > > With `real_path`:
> > > {F7039629}
> > > {F7041680}
> > > 
> > > Wihtou `real_path`:
> > > {F7039630}
> > But with this patch, we are not using real_path anymore, so it can't be the reason for not computing `RealPathName` when not opening the file.  Since the non-real_path method is much more lightweight, would it make a significant difference if we always computed the field?
> > 
> > In the end I don't really mind, I am just looking for a rational explanation to why it's done this way.
> Ohh, sorry that I was confused with the other thread...
> 
> Honestly, I also don't know where all these came from (it was implemented many years ago...). For now, I'm just trying to fix the problem with the safest change, as the old behavior, although confusing, should be pretty stable.  Changing behavior further would likely cause more problems. I would prefer making further change when we are actually cleaning up or redesigning `RealPathName`/`tryGetRealPath`.
Ok, I gave this a try, and it seems that computing absolute paths for unopened files doesn't cause any problem. Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D51159