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:
> > > > > 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}
================
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:
> > > 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.
Was the bug addressed in D48687 actually caused by symlinks though? If want we
want is absolute paths with dotdot cleaned, it should be much cheaper to call
`VFS::makeAbsolutePath` with `remove_dot_dot`.
In general, it's unclear whether clangd should resolve symlink (it might not
always be what users want), and it should probably be a decision made by the
build system integration. I think we would need a much more careful design if
we decide to handle symlinks in clangd.
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
