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:
> > 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 ..
================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:190
- StringRef FileName = File->tryGetRealPathName().empty()
- ? File->getName()
- : File->tryGetRealPathName();
+ StringRef FileName = SM.getFileManager().getCanonicalName(File);
----------------
MrTrillian wrote:
> benlangmuir wrote:
> > Why is this change needed?
> > Why is this change needed?
>
> @benlangmuir We don't want raw `getRealPath`s on Windows because of
> substitute drives and MAX_PATH issues. That is the idea behind this diff. If
> I leave the `tryGetRealPathName` here, I need to change the
> `relative_include.m` test as in this previous diff:
> https://reviews.llvm.org/D154130?id=539683 , which is undesirable.
I wonder if we should just remove `tryGetRealPathName`; it's not actually the
real path in many cases. Anyway, not for this patch, your change here seems
fine.
================
Comment at: llvm/utils/lit/lit/LitConfig.py:192
f = f.f_back.f_back
- file = os.path.abspath(inspect.getsourcefile(f))
+ file = lit.util.abs_path_preserve_drive(inspect.getsourcefile(f))
line = inspect.getlineno(f)
----------------
Why are you changing abspath (here and elsewhere)? I understand why you are
changing realpath -> lit.util.abs_path_preserve_drive, but what's the issue
with bare abspath?
================
Comment at: llvm/utils/lit/lit/TestRunner.py:1282
+ # a leading slash.
+ substitutions.append(("%:" + letter, colonNormalizePath(path)))
----------------
This change drops the `+ ".tmp"` that was previously added to
`%t:regex_replacement` and `%:t`.
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
