[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov abandoned this revision. ppluzhnikov added a comment. This proved to be too hard :-( A smaller change: https://reviews.llvm.org/D126396 fixed one aspect of this problem. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546 + {"include/wordexp.h", ""}, + {"include/x86intrin.h", ""}, + {"include/xlocale.h", ""}, ilya-biryukov wrote: > ppluzhnikov wrote:

[PATCH] D121733: Clean pathnames in FileManager.

benlangmuir added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:218 + llvm::sys::path::remove_dots(CleanFilename, /*remove_dot_dot=*/false); + Filename = CleanFilename; + ppluzhnikov wrote: > kadircet wrote: > > this is actually breaking the

[PATCH] D121733: Clean pathnames in FileManager.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:24 + static auto *Mappings = + new std::array, 645>{{ + {"algorithm", ""}, ppluzhnikov wrote: > ilya-biryukov wrote: > > Don't specify sizes of

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:218 + llvm::sys::path::remove_dots(CleanFilename, /*remove_dot_dot=*/false); + Filename = CleanFilename; + kadircet wrote: > this is actually breaking the [contract of >

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov planned changes to this revision. ppluzhnikov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:24 + static auto *Mappings = + new std::array, 645>{{ + {"algorithm", ""}, ilya-biryukov wrote: >

Re: [PATCH] D121733: Clean pathnames in FileManager.

Please note that this patch still breaks Winx64 tests, and that I marked it as "further changes required" / not ready for review some time ago. On Wed, May 11, 2022 at 10:37 AM Ilya Biryukov via Phabricator < revi...@reviews.llvm.org> wrote: > ilya-biryukov requested changes to this revision. >

[PATCH] D121733: Clean pathnames in FileManager.

kadircet added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:218 + llvm::sys::path::remove_dots(CleanFilename, /*remove_dot_dot=*/false); + Filename = CleanFilename; + this is actually breaking the [contract of

[PATCH] D121733: Clean pathnames in FileManager.

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Please allow some time for the clangd team to review this before landing. Changes to filenames used to cause unintended consequences for us before. We switched to using

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 428693. ppluzhnikov added a comment. Fix clangd test failure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 Files:

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 428293. ppluzhnikov added a comment. Herald added subscribers: usaxena95, kadircet. Fix FIXME in CanonicalIncludesTests.cpp (yet another Winx64 failure). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 428275. ppluzhnikov added a comment. Fix one more Winx64 failure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 Files:

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 427526. ppluzhnikov added a comment. Fix more UNIX and Winx64 failures. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 Files:

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 427460. ppluzhnikov added a comment. Herald added a subscriber: carlosgalvezp. Fix Winx64 `clang-tidy/checkers/google-upgrade-googletest-case.cpp` failure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added a comment. In D121733#3403995 , @rnk wrote: > Ignoring the ".." path components for the moment, is this patch good to go as > is? It doesn't affect that behavior. Besides the test failure, the other thing is considering what to do with

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov added a comment. > Ignoring the ".." path components for the moment, is this patch good to go as > is? It doesn't affect that behavior. This patch still breaks Winx64 `clang-tidy/checkers/google-upgrade-googletest-case.cpp`. The failure log here

[PATCH] D121733: Clean pathnames in FileManager.

rnk added a comment. Ignoring the ".." path components for the moment, is this patch good to go as is? It doesn't affect that behavior. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added a comment. In D121733#3393640 , @dexonsmith wrote: > In D121733#3393552 , @bnbarham > wrote: > >> In D121733#3393546 , @rnk wrote: >> >>> I've been

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added a comment. In D121733#3393552 , @bnbarham wrote: > In D121733#3393546 , @rnk wrote: > >> I've been somewhat afraid to touch this code because of symlinks. Is that >> something we need to worry

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 416633. ppluzhnikov added a comment. More Debian and Win x64 fixes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 Files:

[PATCH] D121733: Clean pathnames in FileManager.

bnbarham added a comment. In D121733#3393546 , @rnk wrote: > I've been somewhat afraid to touch this code because of symlinks. Is that > something we need to worry about? Consider this path: root/symlink/../foo.h. > remove_dots will turn this into

[PATCH] D121733: Clean pathnames in FileManager.

rnk added a comment. I've been somewhat afraid to touch this code because of symlinks. Is that something we need to worry about? Consider this path: root/symlink/../foo.h. remove_dots will turn this into root/foo.h, but if symlink points to some unrelated directory, the .. directory entry

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov planned changes to this revision. ppluzhnikov added a comment. Pending further Win x64 failure cleanup. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added reviewers: mstorsjo, rnk. dexonsmith added subscribers: mstorsjo, rnk. dexonsmith added a comment. FWIW, I quite like the clean up effect that this patch has (assuming we are able to convince ourselves that it's safe/correct for `FileManager` to do this). - Removing the dots

[PATCH] D121733: Clean pathnames in FileManager.

bnbarham added a comment. In D121733#3392968 , @ppluzhnikov wrote: >> There's also some others where I wouldn't expect them to be failing in this >> patch, eg. the ones from `/` -> `{{[/\\]}}`. > > These are failing because `remove_dots`

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov added a comment. > There's also some others where I wouldn't expect them to be failing in this > patch, eg. the ones from `/` -> `{{[/\\]}}`. These are failing because `remove_dots` (un-intuitively) also changes "foo/bar\\baz" to "foo\\bar\\baz" > Are there tests that we can't

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov added a comment. > @ppluzhnikov, can you give more context on how this interacts with > https://reviews.llvm.org/D121658? I had a quick look but it wasn't > immediately obvious. D121658 broke Winx64 test due to "./" vs. ".\\" mismatch. This

[PATCH] D121733: Clean pathnames in FileManager.

bnbarham added a comment. In D121733#3392931 , @dexonsmith wrote: > However, FileManager changes sometimes have odd side effects... and it's > possible that somewhere in clang relies on having `FileManager::getFileRef()` > return precisely the same

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added reviewers: dexonsmith, benlangmuir, bnbarham, arphaman, vsapsai. dexonsmith added a comment. I haven't had time yet to think through the implications of this. At first glance this seems fine/correct/great. The only case I know of where `./` means something is if you're running

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 416330. ppluzhnikov added a comment. Use single quotes in sed -- don't want shell expansion there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 Files:

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov updated this revision to Diff 416255. ppluzhnikov added a comment. Herald added a project: clang-tools-extra. Fix Win x64 failures. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 Files:

[PATCH] D121733: Clean pathnames in FileManager.

ppluzhnikov created this revision. Herald added subscribers: dexonsmith, arphaman. Herald added a project: All. ppluzhnikov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This reduces visual noise ("./foo.h" -> "foo.h") in error messages.