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:
> > > 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`.
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
