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:
> > > > 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.
================
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:
> > > > 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.
> Was the bug addressed in D48687 actually caused by symlinks though?
No, it was caused by non-relative path that included dotdots, I was
extrapolating.
> If want we want is absolute paths with dotdot cleaned, it should be much
> cheaper to call VFS::makeAbsolutePath with remove_dot_dot.
Agreed.
> 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.
Indeed, if those paths end up displayed in the UI, it could be preferrable to
not resolve the symlinks. Using the real_path was kind of the "atomic bomb"
solution to get rid of duplicates.
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
