Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-19 Thread Jan Korous via cfe-commits 
I see. You're right the name sounds slow indeed. Thank you for the explanation!

> On Feb 19, 2019, at 6:43 AM, Nico Weber  wrote:
> 
> I didn't realize it just copied a string. I just saw a call to 
> "fillRealPathName" and the "RealPath" bit sounded slow. From clicking around 
> in http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp#361 
>  it looks 
> like it's not really computing a realpath (for symlinks). It still does quite 
> a bit of string processing if InterndFileName isn't absolute, but that's 
> probably fine. It's still the kind of thing I'd carefully measure since 
> getFile(openFile=false) was performance-sensitive in a certain case iirc (I 
> think when using pch files?)
> 
> On Mon, Feb 18, 2019 at 5:26 PM Jan Korous  > wrote:
> Hi Nico,
> 
> I didn't think it necessary as the change doesn't introduce any interaction 
> with filesystem - it's just copying a string.
> 
> Do you mean it causes a performance regression?
> 
> Thanks.
> 
> Jan
> 
>> On Feb 15, 2019, at 6:15 AM, Nico Weber > > wrote:
>> 
>> Did you do any performance testing to check if this slows down clang?
>> 
>> On Thu, Feb 14, 2019 at 6:02 PM Jan Korous via cfe-commits 
>> mailto:cfe-commits@lists.llvm.org>> wrote:
>> Author: jkorous
>> Date: Thu Feb 14 15:02:35 2019
>> New Revision: 354075
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev 
>> 
>> Log:
>> [clang][FileManager] fillRealPathName even if we aren't opening the file
>> 
>> The pathname wasn't previously filled when the getFile() method was called 
>> with openFile = false.
>> We are caching FileEntry-s in ParsedAST::Includes in clangd and this caused 
>> the problem.
>> 
>> This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All
>> 
>> rdar://47536127 <>
>> 
>> Differential Revision: https://reviews.llvm.org/D58213 
>> 
>> 
>> Modified:
>> cfe/trunk/lib/Basic/FileManager.cpp
>> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>> 
>> Modified: cfe/trunk/lib/Basic/FileManager.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>>  
>> 
>> ==
>> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
>> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>>if (UFE.File) {
>>  if (auto PathName = UFE.File->getName())
>>fillRealPathName(&UFE, *PathName);
>> +  } else if (!openFile) {
>> +// We should still fill the path even if we aren't opening the file.
>> +fillRealPathName(&UFE, InterndFileName);
>>}
>>return &UFE;
>>  }
>> 
>> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>>  
>> 
>> ==
>> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
>> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
>> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>>  }
>> 
>> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
>> +  auto statCache = llvm::make_unique();
>> +  statCache->InjectDirectory("/tmp/abc", 42);
>> +  SmallString<64> Path("/tmp/abc/foo.cpp");
>> +  statCache->InjectFile(Path.str().str().c_str(), 43);
>> +  manager.setStatCache(std::move(statCache));
>> +
>> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
>> +
>> +  ASSERT_TRUE(file != nullptr);
>> +
>> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
>> +}
>> +
>>  } // anonymous namespace
>> 
>> 
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org 
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
>> 
> 

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

Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-19 Thread Nico Weber via cfe-commits 
I didn't realize it just copied a string. I just saw a call to
"fillRealPathName" and the "RealPath" bit sounded slow. From clicking
around in http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp#361
it looks like it's not really computing a realpath (for symlinks). It still
does quite a bit of string processing if InterndFileName isn't absolute,
but that's probably fine. It's still the kind of thing I'd carefully
measure since getFile(openFile=false) was performance-sensitive in a
certain case iirc (I think when using pch files?)

On Mon, Feb 18, 2019 at 5:26 PM Jan Korous  wrote:

> Hi Nico,
>
> I didn't think it necessary as the change doesn't introduce any
> interaction with filesystem - it's just copying a string.
>
> Do you mean it causes a performance regression?
>
> Thanks.
>
> Jan
>
> On Feb 15, 2019, at 6:15 AM, Nico Weber  wrote:
>
> Did you do any performance testing to check if this slows down clang?
>
> On Thu, Feb 14, 2019 at 6:02 PM Jan Korous via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: jkorous
>> Date: Thu Feb 14 15:02:35 2019
>> New Revision: 354075
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev
>> Log:
>> [clang][FileManager] fillRealPathName even if we aren't opening the file
>>
>> The pathname wasn't previously filled when the getFile() method was
>> called with openFile = false.
>> We are caching FileEntry-s in ParsedAST::Includes in clangd and this
>> caused the problem.
>>
>> This fixes an internal test failure in clangd -
>> ClangdTests.GoToInclude.All
>>
>> rdar://47536127
>>
>> Differential Revision: https://reviews.llvm.org/D58213
>>
>> Modified:
>> cfe/trunk/lib/Basic/FileManager.cpp
>> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>>
>> Modified: cfe/trunk/lib/Basic/FileManager.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
>> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>>if (UFE.File) {
>>  if (auto PathName = UFE.File->getName())
>>fillRealPathName(&UFE, *PathName);
>> +  } else if (!openFile) {
>> +// We should still fill the path even if we aren't opening the file.
>> +fillRealPathName(&UFE, InterndFileName);
>>}
>>return &UFE;
>>  }
>>
>> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>>
>> ==
>> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
>> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
>> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>>  }
>>
>> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
>> +  auto statCache = llvm::make_unique();
>> +  statCache->InjectDirectory("/tmp/abc", 42);
>> +  SmallString<64> Path("/tmp/abc/foo.cpp");
>> +  statCache->InjectFile(Path.str().str().c_str(), 43);
>> +  manager.setStatCache(std::move(statCache));
>> +
>> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
>> +
>> +  ASSERT_TRUE(file != nullptr);
>> +
>> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
>> +}
>> +
>>  } // anonymous namespace
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-18 Thread Jan Korous via cfe-commits 
Hi Nico,

I didn't think it necessary as the change doesn't introduce any interaction 
with filesystem - it's just copying a string.

Do you mean it causes a performance regression?

Thanks.

Jan

> On Feb 15, 2019, at 6:15 AM, Nico Weber  wrote:
> 
> Did you do any performance testing to check if this slows down clang?
> 
> On Thu, Feb 14, 2019 at 6:02 PM Jan Korous via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: jkorous
> Date: Thu Feb 14 15:02:35 2019
> New Revision: 354075
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev 
> 
> Log:
> [clang][FileManager] fillRealPathName even if we aren't opening the file
> 
> The pathname wasn't previously filled when the getFile() method was called 
> with openFile = false.
> We are caching FileEntry-s in ParsedAST::Includes in clangd and this caused 
> the problem.
> 
> This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All
> 
> rdar://47536127
> 
> Differential Revision: https://reviews.llvm.org/D58213 
> 
> 
> Modified:
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
> 
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>  
> 
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>if (UFE.File) {
>  if (auto PathName = UFE.File->getName())
>fillRealPathName(&UFE, *PathName);
> +  } else if (!openFile) {
> +// We should still fill the path even if we aren't opening the file.
> +fillRealPathName(&UFE, InterndFileName);
>}
>return &UFE;
>  }
> 
> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>  
> 
> ==
> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>  }
> 
> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
> +  auto statCache = llvm::make_unique();
> +  statCache->InjectDirectory("/tmp/abc", 42);
> +  SmallString<64> Path("/tmp/abc/foo.cpp");
> +  statCache->InjectFile(Path.str().str().c_str(), 43);
> +  manager.setStatCache(std::move(statCache));
> +
> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
> +
> +  ASSERT_TRUE(file != nullptr);
> +
> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
> +}
> +
>  } // anonymous namespace
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 

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

Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-18 Thread Jan Korous via cfe-commits 
Hi all,

Sorry, my bad, I was out on Friday.

Will re-land patch with fixed test shortly.

Jan

> On Feb 15, 2019, at 12:47 PM, Reid Kleckner  wrote:
> 
> Reverted:
> http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/4351 
> 
> On Fri, Feb 15, 2019 at 11:07 AM Galina Kistanova via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Hello Jan,
> 
> It looks like this commit broke tests on couple of win builders:
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/23655
>  
> 
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win 
> 
> . . .
> Failing Tests (1):
> Clang-Unit :: 
> Basic/./BasicTests.exe/FileManagerTest.getFileDontOpenRealPath
> 
> Please have a look ASAP?
> 
> Thanks
> 
> Galina
> 
> On Thu, Feb 14, 2019 at 3:02 PM Jan Korous via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: jkorous
> Date: Thu Feb 14 15:02:35 2019
> New Revision: 354075
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev 
> 
> Log:
> [clang][FileManager] fillRealPathName even if we aren't opening the file
> 
> The pathname wasn't previously filled when the getFile() method was called 
> with openFile = false.
> We are caching FileEntry-s in ParsedAST::Includes in clangd and this caused 
> the problem.
> 
> This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All
> 
> rdar://47536127
> 
> Differential Revision: https://reviews.llvm.org/D58213 
> 
> 
> Modified:
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
> 
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>  
> 
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>if (UFE.File) {
>  if (auto PathName = UFE.File->getName())
>fillRealPathName(&UFE, *PathName);
> +  } else if (!openFile) {
> +// We should still fill the path even if we aren't opening the file.
> +fillRealPathName(&UFE, InterndFileName);
>}
>return &UFE;
>  }
> 
> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>  
> 
> ==
> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>  }
> 
> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
> +  auto statCache = llvm::make_unique();
> +  statCache->InjectDirectory("/tmp/abc", 42);
> +  SmallString<64> Path("/tmp/abc/foo.cpp");
> +  statCache->InjectFile(Path.str().str().c_str(), 43);
> +  manager.setStatCache(std::move(statCache));
> +
> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
> +
> +  ASSERT_TRUE(file != nullptr);
> +
> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
> +}
> +
>  } // anonymous namespace
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 

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

r354169 - Revert r354075 "[clang][FileManager] fillRealPathName even if we aren't opening the file"

2019-02-15 Thread Reid Kleckner via cfe-commits 
Author: rnk
Date: Fri Feb 15 12:48:12 2019
New Revision: 354169

URL: http://llvm.org/viewvc/llvm-project?rev=354169&view=rev
Log:
Revert r354075 "[clang][FileManager] fillRealPathName even if we aren't opening 
the file"

The new test doesn't pass on Windows.

Modified:
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/unittests/Basic/FileManagerTest.cpp

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354169&r1=354168&r2=354169&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Fri Feb 15 12:48:12 2019
@@ -267,9 +267,6 @@ const FileEntry *FileManager::getFile(St
   if (UFE.File) {
 if (auto PathName = UFE.File->getName())
   fillRealPathName(&UFE, *PathName);
-  } else if (!openFile) {
-// We should still fill the path even if we aren't opening the file.
-fillRealPathName(&UFE, InterndFileName);
   }
   return &UFE;
 }

Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354169&r1=354168&r2=354169&view=diff
==
--- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
+++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Fri Feb 15 12:48:12 2019
@@ -346,18 +346,4 @@ TEST_F(FileManagerTest, getVirtualFileFi
   EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
 }
 
-TEST_F(FileManagerTest, getFileDontOpenRealPath) {
-  auto statCache = llvm::make_unique();
-  statCache->InjectDirectory("/tmp/abc", 42);
-  SmallString<64> Path("/tmp/abc/foo.cpp");
-  statCache->InjectFile(Path.str().str().c_str(), 43);
-  manager.setStatCache(std::move(statCache));
-
-  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
-
-  ASSERT_TRUE(file != nullptr);
-
-  ASSERT_EQ(file->tryGetRealPathName(), Path);
-}
-
 } // anonymous namespace


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

Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-15 Thread Reid Kleckner via cfe-commits 
Reverted:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/4351

On Fri, Feb 15, 2019 at 11:07 AM Galina Kistanova via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Hello Jan,
>
> It looks like this commit broke tests on couple of win builders:
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/23655
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win
> . . .
> Failing Tests (1):
> Clang-Unit ::
> Basic/./BasicTests.exe/FileManagerTest.getFileDontOpenRealPath
>
> Please have a look ASAP?
>
> Thanks
>
> Galina
>
> On Thu, Feb 14, 2019 at 3:02 PM Jan Korous via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: jkorous
>> Date: Thu Feb 14 15:02:35 2019
>> New Revision: 354075
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev
>> Log:
>> [clang][FileManager] fillRealPathName even if we aren't opening the file
>>
>> The pathname wasn't previously filled when the getFile() method was
>> called with openFile = false.
>> We are caching FileEntry-s in ParsedAST::Includes in clangd and this
>> caused the problem.
>>
>> This fixes an internal test failure in clangd -
>> ClangdTests.GoToInclude.All
>>
>> rdar://47536127
>>
>> Differential Revision: https://reviews.llvm.org/D58213
>>
>> Modified:
>> cfe/trunk/lib/Basic/FileManager.cpp
>> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>>
>> Modified: cfe/trunk/lib/Basic/FileManager.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
>> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>>if (UFE.File) {
>>  if (auto PathName = UFE.File->getName())
>>fillRealPathName(&UFE, *PathName);
>> +  } else if (!openFile) {
>> +// We should still fill the path even if we aren't opening the file.
>> +fillRealPathName(&UFE, InterndFileName);
>>}
>>return &UFE;
>>  }
>>
>> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>>
>> ==
>> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
>> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
>> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>>  }
>>
>> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
>> +  auto statCache = llvm::make_unique();
>> +  statCache->InjectDirectory("/tmp/abc", 42);
>> +  SmallString<64> Path("/tmp/abc/foo.cpp");
>> +  statCache->InjectFile(Path.str().str().c_str(), 43);
>> +  manager.setStatCache(std::move(statCache));
>> +
>> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
>> +
>> +  ASSERT_TRUE(file != nullptr);
>> +
>> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
>> +}
>> +
>>  } // anonymous namespace
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-15 Thread Galina Kistanova via cfe-commits 
Hello Jan,

It looks like this commit broke tests on couple of win builders:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/23655
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win
. . .
Failing Tests (1):
Clang-Unit ::
Basic/./BasicTests.exe/FileManagerTest.getFileDontOpenRealPath

Please have a look ASAP?

Thanks

Galina

On Thu, Feb 14, 2019 at 3:02 PM Jan Korous via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: jkorous
> Date: Thu Feb 14 15:02:35 2019
> New Revision: 354075
>
> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev
> Log:
> [clang][FileManager] fillRealPathName even if we aren't opening the file
>
> The pathname wasn't previously filled when the getFile() method was called
> with openFile = false.
> We are caching FileEntry-s in ParsedAST::Includes in clangd and this
> caused the problem.
>
> This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All
>
> rdar://47536127
>
> Differential Revision: https://reviews.llvm.org/D58213
>
> Modified:
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>if (UFE.File) {
>  if (auto PathName = UFE.File->getName())
>fillRealPathName(&UFE, *PathName);
> +  } else if (!openFile) {
> +// We should still fill the path even if we aren't opening the file.
> +fillRealPathName(&UFE, InterndFileName);
>}
>return &UFE;
>  }
>
> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>
> ==
> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>  }
>
> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
> +  auto statCache = llvm::make_unique();
> +  statCache->InjectDirectory("/tmp/abc", 42);
> +  SmallString<64> Path("/tmp/abc/foo.cpp");
> +  statCache->InjectFile(Path.str().str().c_str(), 43);
> +  manager.setStatCache(std::move(statCache));
> +
> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
> +
> +  ASSERT_TRUE(file != nullptr);
> +
> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
> +}
> +
>  } // anonymous namespace
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-15 Thread Nico Weber via cfe-commits 
Did you do any performance testing to check if this slows down clang?

On Thu, Feb 14, 2019 at 6:02 PM Jan Korous via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: jkorous
> Date: Thu Feb 14 15:02:35 2019
> New Revision: 354075
>
> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev
> Log:
> [clang][FileManager] fillRealPathName even if we aren't opening the file
>
> The pathname wasn't previously filled when the getFile() method was called
> with openFile = false.
> We are caching FileEntry-s in ParsedAST::Includes in clangd and this
> caused the problem.
>
> This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All
>
> rdar://47536127
>
> Differential Revision: https://reviews.llvm.org/D58213
>
> Modified:
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
>
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
>if (UFE.File) {
>  if (auto PathName = UFE.File->getName())
>fillRealPathName(&UFE, *PathName);
> +  } else if (!openFile) {
> +// We should still fill the path even if we aren't opening the file.
> +fillRealPathName(&UFE, InterndFileName);
>}
>return &UFE;
>  }
>
> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
>
> ==
> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>  }
>
> +TEST_F(FileManagerTest, getFileDontOpenRealPath) {
> +  auto statCache = llvm::make_unique();
> +  statCache->InjectDirectory("/tmp/abc", 42);
> +  SmallString<64> Path("/tmp/abc/foo.cpp");
> +  statCache->InjectFile(Path.str().str().c_str(), 43);
> +  manager.setStatCache(std::move(statCache));
> +
> +  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
> +
> +  ASSERT_TRUE(file != nullptr);
> +
> +  ASSERT_EQ(file->tryGetRealPathName(), Path);
> +}
> +
>  } // anonymous namespace
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

2019-02-14 Thread Jan Korous via cfe-commits 
Author: jkorous
Date: Thu Feb 14 15:02:35 2019
New Revision: 354075

URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev
Log:
[clang][FileManager] fillRealPathName even if we aren't opening the file

The pathname wasn't previously filled when the getFile() method was called with 
openFile = false.
We are caching FileEntry-s in ParsedAST::Includes in clangd and this caused the 
problem.

This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All

rdar://47536127

Differential Revision: https://reviews.llvm.org/D58213

Modified:
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/unittests/Basic/FileManagerTest.cpp

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019
@@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St
   if (UFE.File) {
 if (auto PathName = UFE.File->getName())
   fillRealPathName(&UFE, *PathName);
+  } else if (!openFile) {
+// We should still fill the path even if we aren't opening the file.
+fillRealPathName(&UFE, InterndFileName);
   }
   return &UFE;
 }

Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff
==
--- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
+++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019
@@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi
   EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
 }
 
+TEST_F(FileManagerTest, getFileDontOpenRealPath) {
+  auto statCache = llvm::make_unique();
+  statCache->InjectDirectory("/tmp/abc", 42);
+  SmallString<64> Path("/tmp/abc/foo.cpp");
+  statCache->InjectFile(Path.str().str().c_str(), 43);
+  manager.setStatCache(std::move(statCache));
+
+  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
+
+  ASSERT_TRUE(file != nullptr);
+
+  ASSERT_EQ(file->tryGetRealPathName(), Path);
+}
+
 } // anonymous namespace


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