benlangmuir added inline comments.
================
Comment at: clang/lib/Basic/FileManager.cpp:663
+ } else {
+ llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+ CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);
----------------
MrTrillian wrote:
> benlangmuir wrote:
> > MrTrillian wrote:
> > > benlangmuir wrote:
> > > > MrTrillian wrote:
> > > > > benlangmuir wrote:
> > > > > > Removing .. can change where the path points in the presence of
> > > > > > symlinks; is this needed?
> > > > > > Removing .. can change where the path points in the presence of
> > > > > > symlinks; is this needed?
> > > > >
> > > > > @benlangmuir That's true and not ideal, but `makeAbsolute` will not
> > > > > resolve `/./` or `/../` in paths, so it's not a canonicalization and
> > > > > some tests were failing because of that. One alternative would be to
> > > > > use `makeAbsolute` + `remove_dots` on Windows (where removing dot
> > > > > dots is semantically correct) and `getRealPath` on Unix, like I do in
> > > > > lit. Suggestions?
> > > > Wouldn't removing .. have the same issue with symlinks on Windows? I
> > > > know symlinks are less common there, but it's not clear to me why it
> > > > would be correct. I guess you could also check if the paths resolve to
> > > > the same file after removing ..
> > > >
> > > >
> > > @benlangmuir Windows simplifies `\..\` in paths before starting to walk
> > > the filesystem.
> > > https://superuser.com/questions/1502360/different-behavior-of-double-dots-and-symlinks-in-windows-and-unix
> > >
> > > If I detected that the path didn't resolve to the same file after
> > > removing `..`, what would I do?
> > >
> > > I think the current logic will only end up using the `else` branch on
> > > Windows. At least I can't think of a scenario where the root would change
> > > from using realpath on unix.
> > Thanks, I wasn't aware of that subtlety! I suggest we add a comment about
> > that here and also mention (or assert) that this is only reachable on
> > Windows.
> > Thanks, I wasn't aware of that subtlety! I suggest we add a comment about
> > that here and also mention (or assert) that this is only reachable on
> > Windows.
>
> @benlangmuir Accounted for in lastest revision. The root-preserving
> canonicalization logic is now Windows-only.
Thanks, looks good.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154130/new/
https://reviews.llvm.org/D154130
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
