[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
> > ilya-biryukov wrote:
> > > Why do we change the order of elements here?
> > > Please revert, this increases the diff and is not relevant to the actual 
> > > change.
> > Note that the elements are inserted into a map
> > (after commit b3a991df3cd6a; used to be a vector before).
> > 
> > Also note that there are duplicates, e.g.
> > 
> > {"bits/typesizes.h", ""},
> > {"bits/typesizes.h", ""},
> > 
> > which can't work as intended / is already broken.
> > 
> > Sorting helps to find these duplicates.
> > 
> This refactoring makes sense, but please split this into a separate change.
https://reviews.llvm.org/D125742


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Ben Langmuir via Phabricator via cfe-commits]

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 [contract of 
> > FileEntryRef](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileEntry.h#L58-L59),
> >  is this the point?
> > 
> > ```
> > /// A reference to a \c FileEntry that includes the name of the file as it 
> > was
> > /// accessed by the FileManager's client.
> > ```
> > 
> > I don't see mention of this contract being changed explicitly anywhere, if 
> > so can you mention it in the commit message and also update the 
> > documentation? (the same applies to DirectoryEntryRef changes as well)
> > 
> > I am not able to take a detailed look at this at the moment, but this 
> > doesn't feel like a safe change for all clients. Because people might be 
> > relying on this contract without explicitly testing the behaviour for "./" 
> > in the filenames. So tests passing (especially when you're just updating 
> > them to not have `./`) might not be implying it's safe.
> I chased this comment down to commit 4dc5573acc0d2e7c59d8bac2543eb25cb4b32984.
> 
> The commit message says:
> 
>This commit introduces a parallel API to FileManager's getFile: 
> getFileEntryRef, which returns
> a reference to the FileEntry, and the name that was used to access the 
> file. In the case of
> a VFS with 'use-external-names', the FileEntyRef contains the external 
> name of the file,
> not the filename that was used to access it.
> 
> So it appears that the comment itself is not quite correct.
> 
> It's also a bit ambiguous (I think) -- "name used to access the file"
> could be interpreted as the name which clang itself used to access
> the file, and not necessarily the name client used.
> 
> > people might be relying on this contract without explicitly testing the 
> > behaviour for "./" in the filenames.
> 
> That's a possibility.
> 
> I am not sure how to evaluate the benefit of this patch (avoiding noise 
> everywhere) vs. possibly breaking clients.
While the comment is not currently accurate due to use-external-names, [[ 
https://github.com/llvm/llvm-project/blob/47be07074a73bd469b16af440923e3cf3b6b3f10/clang/lib/Basic/FileManager.cpp#L319
 | the goal is to eliminate that behaviour ]] and have a separate API for 
getting the external names.  It's maybe still fine to eliminate `./` here, I 
don't have a strong opinion.

> It's also a bit ambiguous (I think) -- "name used to access the file"
could be interpreted as the name which clang itself used to access
the file, and not necessarily the name client used.

It means the name passed to getFileRef.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Ilya Biryukov via Phabricator via cfe-commits]

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 arrays explicitly. This is error prone.
> std::array requires size.
> 
> I could use std::vector instead, at the cost of an extra allocation.
Use raw arrays.



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
+  {"include/wordexp.h", ""},
+  {"include/x86intrin.h", ""},
+  {"include/xlocale.h", ""},

ppluzhnikov wrote:
> ilya-biryukov wrote:
> > Why do we change the order of elements here?
> > Please revert, this increases the diff and is not relevant to the actual 
> > change.
> Note that the elements are inserted into a map
> (after commit b3a991df3cd6a; used to be a vector before).
> 
> Also note that there are duplicates, e.g.
> 
> {"bits/typesizes.h", ""},
> {"bits/typesizes.h", ""},
> 
> which can't work as intended / is already broken.
> 
> Sorting helps to find these duplicates.
> 
This refactoring makes sense, but please split this into a separate change.



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
+  }};
+  auto *HeaderMapping = new llvm::StringMap(Mappings->size());
+

ppluzhnikov wrote:
> ilya-biryukov wrote:
> > This line introduces a memory leak.
> > Notice how the previous version had a `static` variable.
> No, it does not. This function is called only once to initialize a static 
> variable: 
> 
> static const auto *SystemHeaderMap = GetHeaderMapping();
>  
There is no guarantee someone won't run this again by mistake in the future 
revisions.
Make this function leak-free, don't rely on global invariants.

It's easy to do with a lambda trick from the next comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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 
> FileEntryRef](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileEntry.h#L58-L59),
>  is this the point?
> 
> ```
> /// A reference to a \c FileEntry that includes the name of the file as it was
> /// accessed by the FileManager's client.
> ```
> 
> I don't see mention of this contract being changed explicitly anywhere, if so 
> can you mention it in the commit message and also update the documentation? 
> (the same applies to DirectoryEntryRef changes as well)
> 
> I am not able to take a detailed look at this at the moment, but this doesn't 
> feel like a safe change for all clients. Because people might be relying on 
> this contract without explicitly testing the behaviour for "./" in the 
> filenames. So tests passing (especially when you're just updating them to not 
> have `./`) might not be implying it's safe.
I chased this comment down to commit 4dc5573acc0d2e7c59d8bac2543eb25cb4b32984.

The commit message says:

   This commit introduces a parallel API to FileManager's getFile: 
getFileEntryRef, which returns
a reference to the FileEntry, and the name that was used to access the 
file. In the case of
a VFS with 'use-external-names', the FileEntyRef contains the external name 
of the file,
not the filename that was used to access it.

So it appears that the comment itself is not quite correct.

It's also a bit ambiguous (I think) -- "name used to access the file"
could be interpreted as the name which clang itself used to access
the file, and not necessarily the name client used.

> people might be relying on this contract without explicitly testing the 
> behaviour for "./" in the filenames.

That's a possibility.

I am not sure how to evaluate the benefit of this patch (avoiding noise 
everywhere) vs. possibly breaking clients.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
> Don't specify sizes of arrays explicitly. This is error prone.
std::array requires size.

I could use std::vector instead, at the cost of an extra allocation.



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
+  {"include/wordexp.h", ""},
+  {"include/x86intrin.h", ""},
+  {"include/xlocale.h", ""},

ilya-biryukov wrote:
> Why do we change the order of elements here?
> Please revert, this increases the diff and is not relevant to the actual 
> change.
Note that the elements are inserted into a map
(after commit b3a991df3cd6a; used to be a vector before).

Also note that there are duplicates, e.g.

{"bits/typesizes.h", ""},
{"bits/typesizes.h", ""},

which can't work as intended / is already broken.

Sorting helps to find these duplicates.




Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
+  }};
+  auto *HeaderMapping = new llvm::StringMap(Mappings->size());
+

ilya-biryukov wrote:
> This line introduces a memory leak.
> Notice how the previous version had a `static` variable.
No, it does not. This function is called only once to initialize a static 
variable: 

static const auto *SystemHeaderMap = GetHeaderMapping();
 



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:677
+  };
+  for (auto  : *Mappings) {
+Canonicalize(p.first);

ilya-biryukov wrote:
> This function is on a critical path. We don't want to pay for `Canonicalize` 
> on every call to it.
> Please create a static variable and initialize in a lambda if that's 
> absolutely necessary.
> ```
> static auto *Mapping = []{ StringMap *Mapping = new ...; /* init code*/ 
> return Mapping; }();
> ```
This function is only called once.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Pluzhnikov via cfe-commits]

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.
> 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 file UIDs since then, but we're still not sure it's
> not going to break us.
>
> Also see other comments, there are a few important issues.
>
>
>
> 
> Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:24
> +  static auto *Mappings =
> +  new std::array, 645>{{
> +  {"algorithm", ""},
> 
> Don't specify sizes of arrays explicitly. This is error prone.
>

std::array requires size.
I could use std::vector instead, at the cost of an extra memory allocation.


> Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
> +  {"include/wordexp.h", ""},
> +  {"include/x86intrin.h", ""},
> +  {"include/xlocale.h", ""},
> 
> Why do we change the order of elements here?
>

Note that the elements are inserted into a map
(after commit b3a991df3cd6a; used to be a vector before).

Also note that there are duplicates, e.g.

{"bits/typesizes.h", ""},
{"bits/typesizes.h", ""},

which can't work as intended / is already broken.

Sorting helps to find these duplicates.



> Please revert, this increases the diff and is not relevant to the actual
> change.
>
>
> 
> Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
> +  }};
> +  auto *HeaderMapping = new
> llvm::StringMap(Mappings->size());
> +
> 
> This line introduces a memory leak.
> Notice how the previous version had a `static` variable.
>

No, it does not. This function is called only once to initialize a static
variable:

static const auto *SystemHeaderMap = GetHeaderMapping();


>
>
> 
> Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:677
> +  };
> +  for (auto  : *Mappings) {
> +Canonicalize(p.first);
> 
> This function is on a critical path.


This function is only called once.



> We don't want to pay for `Canonicalize` on every call to it.
> Please create a static variable and initialize in a lambda if that's
> absolutely necessary.
> ```
> static auto *Mapping = []{ StringMap *Mapping = new ...; /* init code*/
> return Mapping; }();
> ```
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D121733/new/
>
> https://reviews.llvm.org/D121733
>
>

-- 
Paul Pluzhnikov
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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 
FileEntryRef](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileEntry.h#L58-L59),
 is this the point?

```
/// A reference to a \c FileEntry that includes the name of the file as it was
/// accessed by the FileManager's client.
```

I don't see mention of this contract being changed explicitly anywhere, if so 
can you mention it in the commit message and also update the documentation? 
(the same applies to DirectoryEntryRef changes as well)

I am not able to take a detailed look at this at the moment, but this doesn't 
feel like a safe change for all clients. Because people might be relying on 
this contract without explicitly testing the behaviour for "./" in the 
filenames. So tests passing (especially when you're just updating them to not 
have `./`) might not be implying it's safe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Ilya Biryukov via Phabricator via cfe-commits]

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 file UIDs since then, but we're still not sure it's not going 
to break us.

Also see other comments, there are a few important issues.




Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:24
+  static auto *Mappings =
+  new std::array, 645>{{
+  {"algorithm", ""},

Don't specify sizes of arrays explicitly. This is error prone.



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
+  {"include/wordexp.h", ""},
+  {"include/x86intrin.h", ""},
+  {"include/xlocale.h", ""},

Why do we change the order of elements here?
Please revert, this increases the diff and is not relevant to the actual change.



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
+  }};
+  auto *HeaderMapping = new llvm::StringMap(Mappings->size());
+

This line introduces a memory leak.
Notice how the previous version had a `static` variable.



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:677
+  };
+  for (auto  : *Mappings) {
+Canonicalize(p.first);

This function is on a critical path. We don't want to pay for `Canonicalize` on 
every call to it.
Please create a static variable and initialize in a lambda if that's absolutely 
necessary.
```
static auto *Mapping = []{ StringMap *Mapping = new ...; /* init code*/ return 
Mapping; }();
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/test/clang-apply-replacements/conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/modularize/ProblemsMissingHeader.modularize
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/cxx20-hu-04.cpp
  clang/test/Modules/cxx20-hu-05.cpp
  clang/test/Modules/cxx20-hu-06.cpp
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1622,21 +1622,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1659,7 +1660,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1708,7 +1709,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 TEST_F(TransformerTest, GeneratesMetadata) {
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -295,7 +295,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -304,7 +304,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -316,7 +316,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -325,7 +325,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file 

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

Files:
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/test/clang-apply-replacements/conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/modularize/ProblemsMissingHeader.modularize
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/cxx20-hu-04.cpp
  clang/test/Modules/cxx20-hu-05.cpp
  clang/test/Modules/cxx20-hu-06.cpp
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1622,21 +1622,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1659,7 +1660,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1708,7 +1709,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 TEST_F(TransformerTest, GeneratesMetadata) {
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -295,7 +295,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -304,7 +304,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -316,7 +316,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -325,7 +325,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", 

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/modularize/ProblemsMissingHeader.modularize
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/cxx20-hu-04.cpp
  clang/test/Modules/cxx20-hu-05.cpp
  clang/test/Modules/cxx20-hu-06.cpp
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1622,21 +1622,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1659,7 +1660,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1708,7 +1709,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 TEST_F(TransformerTest, GeneratesMetadata) {
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -295,7 +295,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -304,7 +304,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -316,7 +316,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -325,7 +325,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -339,7 +339,7 @@
 )cpp",
   R"(expanded 

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/modularize/ProblemsMissingHeader.modularize
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/cxx20-hu-04.cpp
  clang/test/Modules/cxx20-hu-05.cpp
  clang/test/Modules/cxx20-hu-06.cpp
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1622,21 +1622,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1659,7 +1660,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1708,7 +1709,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 TEST_F(TransformerTest, GeneratesMetadata) {
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -292,7 +292,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -301,7 +301,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -313,7 +313,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -322,7 +322,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -336,7 +336,7 @@
 )cpp",
   R"(expanded tokens:
   a
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

Files:
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/cxx20-hu-04.cpp
  clang/test/Modules/cxx20-hu-05.cpp
  clang/test/Modules/cxx20-hu-06.cpp
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1622,21 +1622,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1659,7 +1660,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1708,7 +1709,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 TEST_F(TransformerTest, GeneratesMetadata) {
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -292,7 +292,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -301,7 +301,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -313,7 +313,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -322,7 +322,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -336,7 +336,7 @@
 )cpp",
   R"(expanded tokens:
   a
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 a # define MACRO ( ) A # B
   mappings:
@@ -401,7 +401,7 @@
   std::string 

[PATCH] D121733: Clean pathnames in FileManager.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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 
`getDirectoryRef()`, which might make sense to update in the same patch to 
avoid inconsistency. See 1st/2nd bullets of 
https://reviews.llvm.org/D121733#3393732.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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 

 doesn't show //why// that test is failing, but I reproduced the failure on my 
home Windows 10 machine, and I am still digging into the failure reason.

One thing I noticed is this diff from running `clang-tidy`:

  < Suppressed 15 warnings (8 in non-user code, 7 with check filters).
  > Suppressed 38 warnings (31 in non-user code, 7 with check filters).

It appears that additional files are treated as "non-user code" after the patch.

However I don't think that's the root cause of the failure: when I added 
`--header-filter='.*'` I got:

  > Suppressed 7 warnings (7 with check filters).

but the test still failed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Reid Kleckner via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D121733#3393640 , @dexonsmith 
wrote:

> 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 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 points 
>>> to something other than root. I can't imagine that people rely on this 
>>> behavior often, but I could be wrong.
>>
>> `remove_dots` doesn't remove `..` by default, it needs to be passed `true` 
>> to do that. This is only removing `.`.
>
> Yeah, I'd be against doing `..`-removal unless we had a cheap-enough way to 
> detect if that was symlink-incorrect and skipped it in that case. But maybe 
> it'd be worth a comment here explicitly saying `..`s were

A couple more thoughts:

- Probably we should change getDirectoryRef at the same time.
- Mabye this cleanup could mostly contained to there (rather than having it in 
both places).
  - Rely on `getDirectoryRef(parent_path(Filename))` to clean up the directory.
  - Construct "clean(er)" filename from `Dir->getName()` and 
`sys::path::filename(Filename))`.
  - Call `remove_leading_dotslash()` in case the dir is `"."`.
  - ... then go down and stat the file.
- I was a bit worried about the hacks for the VFS with 
"use-external-names=true" interfering with my suggestion above, but I looked 
and I think it's okay.
  - Hacks: see logic for when `Status.getName() != Filename`: `Filename` gets 
replaced with what `Status` returned.
  - Seems like `getDirectoryRef()` doesn't have this hack so I don't think 
it'll interfere here.
  - BTW, @bnbarham and I talked through a way to remove this hack (to model 
exposing the external name without replacing the access name); hopefully it 
won't stick around too much longer!
- It's probably not very hard/expensive to do `..`-removal correctly (correctly 
in all cases, by not removing `..`s unless `stat` proves it's safe).
  - Call `getDirectoryRef()` as above.
  - If there are any internal "..", try removing them and call 
`getDirectoryRef()` again. If they're successful and have the same inode, use 
the cleaner name.
  - The extra stat happens only once per directory that has a `..` component. 
Maybe not too bad?
  - ... again, probably better to do this internally to `getDirectoryRef()`
  - ... certainly seems better to land this as a second incremental step, since 
there's more potential for trouble.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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 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 points to something other than 
>> root. I can't imagine that people rely on this behavior often, but I could 
>> be wrong.
>
> `remove_dots` doesn't remove `..` by default, it needs to be passed `true` to 
> do that. This is only removing `.`.

Yeah, I'd be against doing `..`-removal unless we had a cheap-enough way to 
detect if that was symlink-incorrect and skipped it in that case. But maybe 
it'd be worth a comment here explicitly saying `..`s were intentionally not 
being canonicalized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
  clang-tools-extra/test/clang-apply-replacements/conflict.cpp
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1593,21 +1593,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1630,7 +1631,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1679,7 +1680,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 } // namespace
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -292,7 +292,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -301,7 +301,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -313,7 +313,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -322,7 +322,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -336,7 +336,7 @@
 )cpp",
   R"(expanded tokens:
   a
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 a # define MACRO ( ) A # B
   mappings:
@@ -401,7 +401,7 @@
   std::string Expected =
   "expanded tokens:\n"
   "  int a ;\n"
-  "file './input.cpp'\n"
+  "file 'input.cpp'\n"
   "  spelled tokens:\n"
   "# define FOO a # include \"unresolved_file.h\" # undef FOO "
   "# ifdef X # else # endif # ifndef Y # endif # if 1 # elif 2 # else "
@@ 

[PATCH] D121733: Clean pathnames in FileManager.

[Ben Barham via Phabricator via cfe-commits]

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 root/foo.h, but if symlink points to some 
> unrelated directory, the .. directory entry points to something other than 
> root. I can't imagine that people rely on this behavior often, but I could be 
> wrong.

`remove_dots` doesn't remove `..` by default, it needs to be passed `true` to 
do that. This is only removing `.`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Reid Kleckner via Phabricator via cfe-commits]

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 points to something other than 
root. I can't imagine that people rely on this behavior often, but I could be 
wrong.

If the consequences are mostly that diagnostics and  `__FILE__`  in such a case 
refer to non-existent files, I can live with that, it's just a moderate 
reduction in usability for what seems like unreasonable usage.

I have also wanted to do slash canonicalization at this exact point, to respect 
the user setting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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 feels like pure goodness.
- Canonicalizing the slashes also seems potentially nice (I had missed that 
`remove_dots()` does this), as long as `FileManager` is respecting any 
user-specified path style on Windows. Note that the style could be either 
`sys::path::Style::windows_slash` or `sys::path::Style::windows_backslash`, but 
your `remove_dots` call will be following `sys::path::Style::native`. Although, 
it could also be hard to reason about, if some paths in the output are 
trafficked through `FileManager` and get cleaned up, while others are used 
directly and don't get cleaned up.

Given the Windows slash behaviour, would be good to get a Windows reviewer in, 
maybe @mstorsjo or @rnk.

> That's what this change is doing (but there is more Winx64 cleanup pending; 
> sorry, I should have sent this as a draft, but I am still learning the ropes 
> here).

No worries; sorry for jumping in guns blazing. (Drafts are a new feature, and 
I've been afraid to touch them myself :).)

BTW, if this still isn't ready for review, you can mark it as "changes planned".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Ben Barham via Phabricator via cfe-commits]

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` (un-intuitively) also changes 
> "foo/bar\\baz" to "foo\\bar\\baz"
>
>> Are there tests that we can't just fix to expect either `/` or `\\`?
>
> That's what this change is doing (but there is more Winx64 cleanup pending; 
> sorry, I should have sent this as a draft, but I am still learning the ropes 
> here).

Ah, I didn't realise that `remove_dots` also changed slashes.

>> Why do we need to change the underlying behaviour here at all?
>
> Sorry, I didn't understand that comment.

I was referring to the other patch here. I was under the impression that you 
started this patch to fix the failures in D121658 
, but perhaps that isn't the case and you 
just thought this was a generally nice cleanup.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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 just fix to expect either `/` or `\\`?

That's what this change is doing (but there is more Winx64 cleanup pending; 
sorry, I should have sent this as a draft, but I am still learning the ropes 
here).

> Why do we need to change the underlying behaviour here at all?

Sorry, I didn't understand that comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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 change removes "./", so hopefully after this goes in 
D121658  will no longer break anything.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Ben Barham via Phabricator via cfe-commits]

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 path that was requested. Tagging a few other people 
> that have some context... please share your opinions!

Wouldn't that already not be the case though (ie. given `RedirectingFileSystem` 
and `use-external-names` is currently a thing)? I know we're wanting to change 
that, but I don't *know* of anywhere that depends on this currently.

> @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.

If I understand correctly, the failing tests in that patch are failing because 
they're always expecting "/" and since `sys::append` is used, it's now "\\" on 
Windows. The remove dots change doesn't fix those, since the tests would still 
need to be updated to remove the "./" (which is most of the tests in this 
patch). But there's also some others where I wouldn't expect them to be failing 
in this patch, eg. the ones from `/` -> `{{[/\\]}}`.

Are there tests that we can't just fix to expect either `/` or `\\`? Why do we 
need to change the underlying behaviour here at all?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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 an executable, some shells block 
finding executables in the current path without it. I don't think the 
FileManager is used for cleaning up relative paths to executables (in the 
unlikely case such a usecase grows, the caller can surely handle this!).

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 path that was requested. Tagging a few other people 
that have some context... please share your opinions!

@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121733/new/

https://reviews.llvm.org/D121733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1593,21 +1593,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1630,7 +1631,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1679,7 +1680,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 } // namespace
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -292,7 +292,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -301,7 +301,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -313,7 +313,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -322,7 +322,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -336,7 +336,7 @@
 )cpp",
   R"(expanded tokens:
   a
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 a # define MACRO ( ) A # B
   mappings:
@@ -401,7 +401,7 @@
   std::string Expected =
   "expanded tokens:\n"
   "  int a ;\n"
-  "file './input.cpp'\n"
+  "file 'input.cpp'\n"
   "  spelled tokens:\n"
   "# define FOO a # include \"unresolved_file.h\" # undef FOO "
   "# ifdef X # else # endif # ifndef Y # endif # if 1 # elif 2 # else "
@@ -420,7 +420,7 @@
   )cpp",
R"(expanded tokens:
   int const a ;
-file './input.cpp'

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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:
  clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1593,21 +1593,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1630,7 +1631,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1679,7 +1680,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 } // namespace
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -292,7 +292,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -301,7 +301,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -313,7 +313,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -322,7 +322,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -336,7 +336,7 @@
 )cpp",
   R"(expanded tokens:
   a
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 a # define MACRO ( ) A # B
   mappings:
@@ -401,7 +401,7 @@
   std::string Expected =
   "expanded tokens:\n"
   "  int a ;\n"
-  "file './input.cpp'\n"
+  "file 'input.cpp'\n"
   "  spelled tokens:\n"
   "# define FOO a # include \"unresolved_file.h\" # undef FOO "
   "# ifdef X # else # endif # ifndef Y # endif # if 1 # elif 2 # else "
@@ -420,7 +420,7 @@
   )cpp",
R"(expanded tokens:
   int const a ;
-file 

[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

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.

Fix all tests which fail as a result.

This also fixes a class of failures when __FILE__ is used in modules, and
the module specifies 'textual header "foo.h"' but Clang has "./foo.h"
included elsewhere.

Finally this should fix Windows failure from https://reviews.llvm.org/D121658


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121733

Files:
  clang/bindings/python/tests/cindex/test_translation_unit.py
  clang/lib/Basic/FileManager.cpp
  clang/test/Frontend/dependency-gen.c
  clang/test/Index/skip-parsed-bodies/compile_commands.json
  clang/test/Modules/filename.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1593,21 +1593,22 @@
   return L.getFilePath() < R.getFilePath();
 });
 
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
-  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
+  const auto _h = Changes[1];
+  ASSERT_EQ(change_h.getFilePath(), "input.h");
+  EXPECT_THAT(change_h.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_h.getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
-  clang::tooling::applyAllReplacements(Header,
-   Changes[0].getReplacements());
+  clang::tooling::applyAllReplacements(Header, change_h.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), "");
 
-  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
-  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
-  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
-  UpdatedCode = clang::tooling::applyAllReplacements(
-  Source, Changes[1].getReplacements());
+  const auto _cc = Changes[0];
+  ASSERT_EQ(change_cc.getFilePath(), "input.cc");
+  EXPECT_THAT(change_cc.getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(change_cc.getRemovedHeaders(), IsEmpty());
+  UpdatedCode =
+  clang::tooling::applyAllReplacements(Source, change_cc.getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
@@ -1630,7 +1631,7 @@
   {{"input.h", Header}}));
 
   ASSERT_EQ(Changes.size(), 1U);
-  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  ASSERT_EQ(Changes[0].getFilePath(), "input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected UpdatedCode =
@@ -1679,7 +1680,7 @@
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
"input.cc"),
   ResultOf([](const AtomicChange ) { return C.getFilePath(); },
-   "./input.h";
+   "input.h";
 }
 
 } // namespace
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp
===
--- clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -292,7 +292,7 @@
   {"int main() {}",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -301,7 +301,7 @@
   {"\t\n  int\t\n  main\t\n  (\t\n  )\t\n{\t\n  }\t\n",
R"(expanded tokens:
   int main ( ) { }
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 int main ( ) { }
   no mappings.
@@ -313,7 +313,7 @@
   )cpp",
R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 # pragma GCC visibility push ( public ) # pragma GCC visibility pop
   mappings:
@@ -322,7 +322,7 @@
   // Empty files should not crash.
   {R"cpp()cpp", R"(expanded tokens:
   
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 
   no mappings.
@@ -336,7 +336,7 @@
 )cpp",
   R"(expanded tokens:
   a
-file './input.cpp'
+file 'input.cpp'
   spelled tokens:
 a # define MACRO ( ) A # B
   mappings:
@@ -401,7 +401,7 @@
   std::string Expected =
   "expanded tokens:\n"
   "  int a ;\n"
-  "file './input.cpp'\n"
+  "file 'input.cpp'\n"
   "  spelled tokens:\n"
   "# define FOO a # include \"unresolved_file.h\" # undef FOO "
   "# ifdef X #