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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
