MrTrillian 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);
----------------
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.
================
Comment at: llvm/utils/lit/lit/TestRunner.py:1282
+ # a leading slash.
+ substitutions.append(("%:" + letter, colonNormalizePath(path)))
----------------
benlangmuir wrote:
> This change drops the `+ ".tmp"` that was previously added to
> `%t:regex_replacement` and `%:t`.
> This change drops the `+ ".tmp"` that was previously added to
> `%t:regex_replacement` and `%:t`.
@benlangmuir Actually not! The `path_substitutions` list uses `("t", tmpName)`
which includes the `.tmp`, instead of `tmpBase` which doesn't. Looking at the
original code, there didn't seem to be a reason to use `tmpBase` and re-append
`.tmp` instead of using `tmpName`.
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
