[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGecb00a77624c: [VFS] Add support to RedirectingFileSystem for 
mapping a virtual directory to… (authored by nathawes).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94844

Files:
  clang/test/VFS/Inputs/vfsoverlay-directory-relative.yaml
  clang/test/VFS/Inputs/vfsoverlay-directory.yaml
  clang/test/VFS/directory.c
  lldb/source/Host/common/FileSystem.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1328,6 +1328,7 @@
 
 TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -1343,6 +1344,17 @@
   "  'type': 'file',\n"
   "  'name': 'file2',\n"
   "  'external-contents': '//root/foo/b'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir',\n"
+  "  'external-contents': '//root/foo/bar'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir2',\n"
+  "  'use-external-name': false,\n"
+  "  'external-contents': '//root/foo/bar'\n"
   "}\n"
   "  ]\n"
   "}\n"
@@ -1380,12 +1392,221 @@
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
+  // remapped directory
+  S = O->status("//root/mappeddir");
+  ASSERT_FALSE(S.getError());
+  EXPECT_TRUE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
+
+  SLower = O->status("//root/foo/bar");
+  EXPECT_EQ("//root/foo/bar", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file in remapped directory
+  S = O->status("//root/mappeddir/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/foo/bar/a", S->getName());
+
+  // file in remapped directory, with use-external-name=false
+  S = O->status("//root/mappeddir2/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+
+  // file contents in remapped directory
+  OpenedF = O->openFileForRead("//root/mappeddir/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  // file contents in remapped directory, with use-external-name=false
+  OpenedF = O->openFileForRead("//root/mappeddir2/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
 llvm::errc::no_such_file_or_directory);
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, MappedRoot) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{ 'roots': [\n"
+"{\n"
+"  'type': 'directory-remap',\n"
+"  'name': '//mappedroot/',\n"
+"  'external-contents': '//root/foo/bar'\n"
+"}\n"
+"]\n"
+"}",
+Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  IntrusiveRefCntPtr O(
+  new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
+
+  // file
+  ErrorOr S = O->status("//mappedroot/a");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  ErrorOr SLower = O->status("//root/foo/bar/a");
+  EXPECT_EQ("//root/foo/bar/a", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file after opening
+  auto OpenedF = O->openFileForRead("//mappedroot/a");
+  

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 320646.
nathawes added a comment.

Made the following changes as a speculative fix for the windows test failures 
in the pre-merge checks:

- When appending the remaining path components to the external redirect path of 
a directory-remap entry, use the separator type of the redirect path rather 
than the host system.
- For a directory-remap entry, when changing the paths returned in the external 
file system's directory iterator to appear to be in the virtual file system's 
directory, use the separator type from the virtual directory's path rather than 
the host system's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94844

Files:
  clang/test/VFS/Inputs/vfsoverlay-directory-relative.yaml
  clang/test/VFS/Inputs/vfsoverlay-directory.yaml
  clang/test/VFS/directory.c
  lldb/source/Host/common/FileSystem.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1328,6 +1328,7 @@
 
 TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -1343,6 +1344,17 @@
   "  'type': 'file',\n"
   "  'name': 'file2',\n"
   "  'external-contents': '//root/foo/b'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir',\n"
+  "  'external-contents': '//root/foo/bar'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir2',\n"
+  "  'use-external-name': false,\n"
+  "  'external-contents': '//root/foo/bar'\n"
   "}\n"
   "  ]\n"
   "}\n"
@@ -1380,12 +1392,221 @@
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
+  // remapped directory
+  S = O->status("//root/mappeddir");
+  ASSERT_FALSE(S.getError());
+  EXPECT_TRUE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
+
+  SLower = O->status("//root/foo/bar");
+  EXPECT_EQ("//root/foo/bar", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file in remapped directory
+  S = O->status("//root/mappeddir/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/foo/bar/a", S->getName());
+
+  // file in remapped directory, with use-external-name=false
+  S = O->status("//root/mappeddir2/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+
+  // file contents in remapped directory
+  OpenedF = O->openFileForRead("//root/mappeddir/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  // file contents in remapped directory, with use-external-name=false
+  OpenedF = O->openFileForRead("//root/mappeddir2/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
 llvm::errc::no_such_file_or_directory);
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, MappedRoot) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{ 'roots': [\n"
+"{\n"
+"  'type': 'directory-remap',\n"
+"  'name': '//mappedroot/',\n"
+"  'external-contents': '//root/foo/bar'\n"
+"}\n"
+"]\n"
+"}",
+Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  IntrusiveRefCntPtr O(
+  new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
+
+  // file
+  ErrorOr S = O->status("//mappedroot/a");
+  ASSERT_FALSE(S.getError());
+  

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 320257.
nathawes marked an inline comment as done.
nathawes edited the summary of this revision.
nathawes set the repository for this revision to rG LLVM Github Monorepo.
nathawes added a comment.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- Updated shouldFallBackToExternalFS() to accept an Entry * rather than an 
Optional to avoid an unnecessary copy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94844

Files:
  clang/test/VFS/Inputs/vfsoverlay-directory-relative.yaml
  clang/test/VFS/Inputs/vfsoverlay-directory.yaml
  clang/test/VFS/directory.c
  lldb/source/Host/common/FileSystem.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1328,6 +1328,7 @@
 
 TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -1343,6 +1344,17 @@
   "  'type': 'file',\n"
   "  'name': 'file2',\n"
   "  'external-contents': '//root/foo/b'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir',\n"
+  "  'external-contents': '//root/foo/bar'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir2',\n"
+  "  'use-external-name': false,\n"
+  "  'external-contents': '//root/foo/bar'\n"
   "}\n"
   "  ]\n"
   "}\n"
@@ -1380,12 +1392,221 @@
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
+  // remapped directory
+  S = O->status("//root/mappeddir");
+  ASSERT_FALSE(S.getError());
+  EXPECT_TRUE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
+
+  SLower = O->status("//root/foo/bar");
+  EXPECT_EQ("//root/foo/bar", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file in remapped directory
+  S = O->status("//root/mappeddir/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/foo/bar/a", S->getName());
+
+  // file in remapped directory, with use-external-name=false
+  S = O->status("//root/mappeddir2/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+
+  // file contents in remapped directory
+  OpenedF = O->openFileForRead("//root/mappeddir/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  // file contents in remapped directory, with use-external-name=false
+  OpenedF = O->openFileForRead("//root/mappeddir2/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
 llvm::errc::no_such_file_or_directory);
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, MappedRoot) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{ 'roots': [\n"
+"{\n"
+"  'type': 'directory-remap',\n"
+"  'name': '//mappedroot/',\n"
+"  'external-contents': '//root/foo/bar'\n"
+"}\n"
+"]\n"
+"}",
+Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  IntrusiveRefCntPtr O(
+  new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
+
+  // file
+  ErrorOr S = O->status("//mappedroot/a");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  ErrorOr SLower = O->status("//root/foo/bar/a");
+  EXPECT_EQ("//root/foo/bar/a", SLower->getName());
+  

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes marked 6 inline comments as done.
nathawes added a comment.

@JDevlieghere and @dexonsmith would you mind taking another look?

I ended up changing the 'lookup' functions to drop the ExternalRedirect out 
param and supply that via a new 'LookupResult' return value that also gives the 
matched Entry. Hopefully that avoids any confusion over where it's computed vs 
used. It also served as a good home for the duplicated code and the logic in 
the old SetExternalRedirect closure, as Jonas had mentioned. While adding the 
extra directory iteration tests Duncan suggested I noticed I wasn't respecting 
the 'use-external-names' setting in the return items, so I've also added a new 
directory iterator implementation (RedirectingFSDirRemapIterImpl). That one's 
used to wrap the iterator the external file system gives for the directory 
being mapped to, and updates the paths in the items it returns to refer to the 
virtual directory's path instead, before passing them along.


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

https://reviews.llvm.org/D94844

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

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 319505.
nathawes added a comment.

- Updated the yaml for the new kind of virtual directory (that maps to a 
directory in the external filesystem) to have a distinct value for its 'type' 
field ('directory-remap') to better distinguish it from the existing 
'directory' type that behaves quite differently.
- Updated lookupPath to return a structure giving both the matched Entry and 
the ExternalRedirect path, rather than returning the matched Entry and setting 
the ExternalRedirect path via an out parameter. This makes it clearer where the 
ExternalRedirect path is computed vs used.
- Updated RedirectingFileSystem::dir_begin to respect the 'use-external-names' 
setting for DirectoryRemapEntry.
- Added unit tests covering directory iteration (dir_begin) with 
DirectoryRemapEntry support.
- Expanded on the documentation comments for RedirectingFileSystem to hopefully 
make the difference between 'directory' and 'directory-remap' entries clearer.


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

https://reviews.llvm.org/D94844

Files:
  clang/test/VFS/Inputs/vfsoverlay-directory-relative.yaml
  clang/test/VFS/Inputs/vfsoverlay-directory.yaml
  clang/test/VFS/directory.c
  lldb/source/Host/common/FileSystem.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1328,6 +1328,7 @@
 
 TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -1343,6 +1344,17 @@
   "  'type': 'file',\n"
   "  'name': 'file2',\n"
   "  'external-contents': '//root/foo/b'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir',\n"
+  "  'external-contents': '//root/foo/bar'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory-remap',\n"
+  "  'name': 'mappeddir2',\n"
+  "  'use-external-name': false,\n"
+  "  'external-contents': '//root/foo/bar'\n"
   "}\n"
   "  ]\n"
   "}\n"
@@ -1380,12 +1392,221 @@
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
+  // remapped directory
+  S = O->status("//root/mappeddir");
+  ASSERT_FALSE(S.getError());
+  EXPECT_TRUE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
+
+  SLower = O->status("//root/foo/bar");
+  EXPECT_EQ("//root/foo/bar", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file in remapped directory
+  S = O->status("//root/mappeddir/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/foo/bar/a", S->getName());
+
+  // file in remapped directory, with use-external-name=false
+  S = O->status("//root/mappeddir2/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+
+  // file contents in remapped directory
+  OpenedF = O->openFileForRead("//root/mappeddir/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  // file contents in remapped directory, with use-external-name=false
+  OpenedF = O->openFileForRead("//root/mappeddir2/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
 llvm::errc::no_such_file_or_directory);
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, MappedRoot) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{ 'roots': [\n"
+"{\n"
+"  'type': 'directory-remap',\n"
+"  'name': '//mappedroot/',\n"
+"  'external-contents': '//root/foo/bar'\n"
+"}\n"

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes planned changes to this revision.
nathawes added a comment.

Thanks for reviewing @dexonsmith!

> I don't see a test for a case where the mapped directory already exists in 
> the external FS ... Unless I just missed it, can you add one?

I believe the `clang/test/VFS/directory.c` regression test covers that case. In 
that one the real file system is initially set up as follows:

  %t/Underlying/
  C.h
  B.h
  %t/Middle
  C.h
  %t/Overlay
  C.h

And it then sets up vfs overlay yaml files with fallthrough set true that 
redirect Underlying -> Overlay, or Underlying -> Middle and Middle -> Overlay, 
as specified on the numbered lines in the test (e.g. `// 1) Underlying -> 
Overlay`).

> My intuition is we'd want to overlay the directory contents, not replace them

I think it behaves as you expect (as an overlay, rather than replacement) when 
fallthrough is set to true. You can see that happening in first case in the 
same test (`clang/test/VFS/directory.c`). It has the real file system set up as 
above and creates a vfs yaml file that maps `%t/Underlying` to `%t/Overlay` in 
the external/real file system with fallthrough set true. It invokes clang 
passing that overlay file, an include path of `%t/Underlying`, and a source 
file that includes both C.h and B.h,  and checks that it finds the C.h from 
Overlay (after redirecting Underlying -> Overlay) and B.h from Underlying 
(after redirecting Underlying -> Overlay, not finding it, and falling back to 
the original path in the underlying file system).

I really should have a unit test showing the directory iterator output 
directly, though. That makes the overlay behavior much more obvious.


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

https://reviews.llvm.org/D94844

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

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 317157.
nathawes edited the summary of this revision.
nathawes added a comment.

Moved the change combining OverlayFSDirIterImpl and VFSFromYamlDirIterImpl in a 
single implementation into a separate NFC patch 
(https://reviews.llvm.org/D94857)


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

https://reviews.llvm.org/D94844

Files:
  clang/test/VFS/Inputs/vfsoverlay-directory-relative.yaml
  clang/test/VFS/Inputs/vfsoverlay-directory.yaml
  clang/test/VFS/directory.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1328,6 +1328,7 @@
 
 TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -1343,6 +1344,17 @@
   "  'type': 'file',\n"
   "  'name': 'file2',\n"
   "  'external-contents': '//root/foo/b'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': 'mappeddir',\n"
+  "  'external-contents': '//root/foo/bar'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': 'mappeddir2',\n"
+  "  'use-external-name': false,\n"
+  "  'external-contents': '//root/foo/bar'\n"
   "}\n"
   "  ]\n"
   "}\n"
@@ -1374,18 +1386,102 @@
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
 
-  // directory
+  // virtual directory
   S = O->status("//root/");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
+  // mapped directory
+  S = O->status("//root/mappeddir");
+  ASSERT_FALSE(S.getError());
+  EXPECT_TRUE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
+
+  SLower = O->status("//root/foo/bar");
+  EXPECT_EQ("//root/foo/bar", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file in mapped directory
+  S = O->status("//root/mappeddir/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/foo/bar/a", S->getName());
+
+  // file in mapped directory, with use-external-name=false
+  S = O->status("//root/mappeddir2/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+
+  // file contents in mapped directory
+  OpenedF = O->openFileForRead("//root/mappeddir/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  // file contents in mapped directory, with use-external-name=false
+  OpenedF = O->openFileForRead("//root/mappeddir2/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
 llvm::errc::no_such_file_or_directory);
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, MappedRoot) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{ 'roots': [\n"
+"{\n"
+"  'type': 'directory',\n"
+"  'name': '//mappedroot/',\n"
+"  'external-contents': '//root/foo/bar'\n"
+"}\n"
+"]\n"
+"}",
+Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  IntrusiveRefCntPtr O(
+  new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
+
+  // file
+  ErrorOr S = O->status("//mappedroot/a");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->IsVFSMapped);
+
+  ErrorOr SLower = O->status("//root/foo/bar/a");
+  EXPECT_EQ("//root/foo/bar/a", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // 

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes planned changes to this revision.
nathawes added a comment.

Yeah, certainly. Let me put that up separately and update this diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94844

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

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Nathan Hawes via Phabricator via cfe-commits]

nathawes created this revision.
nathawes added a reviewer: JDevlieghere.
Herald added subscribers: dexonsmith, hiraditya.
nathawes requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Previously file entries in the -ivfsoverlay yaml could map to a file in the 
external file system (specified via the 'external-contents' property, taking a 
path), but directories had to list their contents in the form of other file or 
directory entries (via the 'contents' property, taking an array). Allowing 
directory entries to map to a directory in the external file system (via the 
same 'external-contents' property as file entries) makes it possible to present 
an external directory's contents in a different location and, in combination 
with the 'fallthrough' option, overlay one directory's contents on top of 
another.

As an example, with the yaml below:

  { 'roots': [
{
  'type': 'directory',
  'name': '//root/',
  'contents': [
{
  'type': 'directory',
  'name': 'mappeddir',
  'external-contents': '//root/foo/bar'
}
  ]
}
  ]}

The path '//root/mappeddir/somefile' would map to '//root/foo/bar/somefile' in 
the external file system. If that file wasn't found in the external file system 
and the 'fallthrough' configuration option is true, it would fall back to 
checking for '//root/mappeddir/somefile' in the external file system. This 
effectively overlays the contents of //root/foo/bar/ on top of //root/mappeddir.

  

This patch also refactors OverlayFileSystem's directory iterator implementation 
(OverlayFSDirIterImpl) and VFSFromYamlDirIterImpl into a single implementation, 
addressing a FIXME about their conceptual similarity.

Resolves rdar://problem/72485443


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94844

Files:
  clang/test/VFS/Inputs/vfsoverlay-directory-relative.yaml
  clang/test/VFS/Inputs/vfsoverlay-directory.yaml
  clang/test/VFS/directory.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1328,6 +1328,7 @@
 
 TEST_F(VFSFromYAMLTest, MappedFiles) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -1343,6 +1344,17 @@
   "  'type': 'file',\n"
   "  'name': 'file2',\n"
   "  'external-contents': '//root/foo/b'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': 'mappeddir',\n"
+  "  'external-contents': '//root/foo/bar'\n"
+  "},\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': 'mappeddir2',\n"
+  "  'use-external-name': false,\n"
+  "  'external-contents': '//root/foo/bar'\n"
   "}\n"
   "  ]\n"
   "}\n"
@@ -1374,18 +1386,102 @@
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
 
-  // directory
+  // virtual directory
   S = O->status("//root/");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->equivalent(*O->status("//root/"))); // non-volatile UniqueID
 
+  // mapped directory
+  S = O->status("//root/mappeddir");
+  ASSERT_FALSE(S.getError());
+  EXPECT_TRUE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
+
+  SLower = O->status("//root/foo/bar");
+  EXPECT_EQ("//root/foo/bar", SLower->getName());
+  EXPECT_TRUE(S->equivalent(*SLower));
+  EXPECT_FALSE(SLower->IsVFSMapped);
+
+  // file in mapped directory
+  S = O->status("//root/mappeddir/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/foo/bar/a", S->getName());
+
+  // file in mapped directory, with use-external-name=false
+  S = O->status("//root/mappeddir2/a");
+  ASSERT_FALSE(S.getError());
+  ASSERT_FALSE(S->isDirectory());
+  ASSERT_TRUE(S->IsVFSMapped);
+  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+
+  // file contents in mapped directory
+  OpenedF = O->openFileForRead("//root/mappeddir/a");
+  ASSERT_FALSE(OpenedF.getError());
+  OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  // file contents in mapped directory, with 

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Nathan Hawes via Phabricator via cfe-commits]

nathawes accepted this revision.
nathawes added a comment.
This revision is now accepted and ready to land.

Thank you!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58986: Fix typo in string returned from index::getSymbolKindString for SymbolKind::ConversionFunction

[Nathan Hawes via Phabricator via cfe-commits]

nathawes created this revision.
nathawes added a reviewer: akyrtzi.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

This patch updates  SymbolKind::ConversionFunction's string from 
"coversion-func" to "conversion-func" and extends the 
Index/Core/index-source.cpp test to cover the indexing of conversion functions. 
Also changed a few repeated explicit USR matches in that test to refer to 
previous matches instead.


Repository:
  rC Clang

https://reviews.llvm.org/D58986

Files:
  lib/Index/IndexSymbol.cpp
  test/Index/Core/index-source.cpp

Index: test/Index/Core/index-source.cpp
===
--- test/Index/Core/index-source.cpp
+++ test/Index/Core/index-source.cpp
@@ -1,21 +1,27 @@
 // RUN: c-index-test core -print-source-symbols -- %s -std=c++1z -target x86_64-apple-macosx10.7 | FileCheck %s
 // RUN: c-index-test core -print-source-symbols -include-locals -- %s -std=c++1z -target x86_64-apple-macosx10.7 | FileCheck -check-prefix=LOCAL %s
 
+// CHECK: [[@LINE+1]]:7 | class/C++ | Other | [[Other_USR:.*]] |  | Def | rel: 0
+class Other {};
+
 // CHECK: [[@LINE+1]]:7 | class/C++ | Cls | [[Cls_USR:.*]] |  | Def | rel: 0
 class Cls { public:
   // CHECK: [[@LINE+3]]:3 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Decl,RelChild | rel: 1
-  // CHECK-NEXT: RelChild | Cls | c:@S@Cls
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK-NEXT: RelChild | Cls | [[Cls_USR]]
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
   Cls(int x);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-copy-ctor/C++ | Cls | c:@S@Cls@F@Cls#&1$@S@Cls# | __ZN3ClsC1ERKS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
   Cls(const Cls &);
   // CHECK: [[@LINE+2]]:3 | constructor/cxx-move-ctor/C++ | Cls | c:@S@Cls@F@Cls#&&$@S@Cls# | __ZN3ClsC1EOS_ | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:3 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
   Cls(Cls &&);
+  // CHECK: [[@LINE+2]]:3 | conversion-func/C++ | operator Other | [[ClsOtherConv_USR:c:@S@Cls@F@operator Other#]] | [[ClsOtherConv_CGName:.*]] | Def,RelChild | rel: 1
+  // CHECK: [[@LINE+1]]:12 | class/C++ | Other | [[Other_USR]] |  | Ref,RelCont | rel: 1
+  operator Other() { return Other(); }
 
   // CHECK: [[@LINE+2]]:3 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Decl,RelChild | rel: 1
-  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+  // CHECK: [[@LINE+1]]:4 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
   ~Cls();
 };
 
@@ -34,13 +40,20 @@
 
 Cls::Cls(int x) {}
 // CHECK: [[@LINE-1]]:6 | constructor/C++ | Cls | c:@S@Cls@F@Cls#I# | __ZN3ClsC1Ei | Def,RelChild | rel: 1
-// CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-2]]:1 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-3]]:6 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
 
 Cls::~/*a comment*/Cls() {}
 // CHECK: [[@LINE-1]]:6 | destructor/C++ | ~Cls | c:@S@Cls@F@~Cls# | __ZN3ClsD1Ev | Def,RelChild | rel: 1
-// CHECK: [[@LINE-2]]:1 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
-// CHECK: [[@LINE-3]]:20 | class/C++ | Cls | c:@S@Cls |  | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-2]]:1 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-3]]:20 | class/C++ | Cls | [[Cls_USR]] |  | Ref,RelCont | rel: 1
+
+void testConversion() {
+  Cls foo(2);
+
+  // CHECK: [[@LINE+1]]:15 | conversion-func/C++ | operator Other | [[ClsOtherConv_USR]] | [[ClsOtherConv_CGName]] | Ref,Call,RelCall,RelCont | rel: 1
+  Other bar = foo;
+}
 
 template 
 class TemplCls {
@@ -298,17 +311,17 @@
 // CHECK: [[@LINE-1]]:7 | class(Gen,TPS)/C++ | PartialSpecilizationClass | c:@SP>1#T@PartialSpecilizationClass>#$@S@Cls#t0.0 |  | Decl,RelSpecialization | rel: 1
 // CHECK-NEXT: RelSpecialization | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass
 // CHECK: [[@LINE-3]]:7 | class(Gen)/C++ | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass |  | Ref | rel: 0
-// CHECK-NEXT: [[@LINE-4]]:33 | class/C++ | Cls | c:@S@Cls |  | Ref | rel: 0
+// CHECK-NEXT: [[@LINE-4]]:33 | class/C++ | Cls | [[Cls_USR]] |  | Ref | rel: 0
 
 template<>
 class PartialSpecilizationClass : Cls { };
 // CHECK: [[@LINE-1]]:7 | class(Gen,TS)/C++ | PartialSpecilizationClass | c:@S@PartialSpecilizationClass>#$@S@Cls#S0_ |  | Def,RelSpecialization | rel: 1
 // CHECK-NEXT: RelSpecialization | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass
-// CHECK-NEXT: [[@LINE-3]]:45 | class/C++ | Cls | 

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added a comment.
Herald added a subscriber: ormris.

In D58814#1415607 , @gribozavr wrote:

> > These references were added to support using the index data for symbol 
> > rename.
>
> Understood -- thank you for providing the context (the original commit that 
> added this code didn't have the explanation why this reference was added).


That was my patch (via Argyrios), so sorry about that!

> However, my suggestion would be to still take this patch.  For symbol rename, 
> my suggestion would be one of the following.
> 
> Option (1): Symbol rename is a complex operation that requires knowing many 
> places where the class name appears.  So I think it is fair that it would 
> need to navigate to all such declarations using the semantic index -- find 
> the class, then find its constructors, destructor, out-of-line functions, 
> find derived classes, then find `using` declarations in derived classes, find 
> calls to constructors, destructor etc.  I don't think it is not the job of 
> the core indexer API to hardcode all of these places as a "reference to the 
> class".  Different clients require different navigation to related symbols, 
> and hardcoding all such navigation patterns in the indexer would create a 
> messy index that is difficult to work with, since you'd have to filter out 
> lots of stuff.  Not to even mention the index size.

I feel like the complexity you mention here is what makes it worthwhile to 
expose all these locations as occurrences of the type, and I'm fairly sure 
we're actually already doing that in all these locations (unless I've missed 
other patches removing them). Pushing this work onto all clients that want to 
provide automatic global rename functionality or to support users manually 
renaming by including these in their find-references results seems like a 
poorer outcome from a code sharing/maintenance point of view than continuing to 
provide it and requiring clients that don't want it to check the SymbolRole 
bits inside their IndexDataConsumer's handleDeclOccurrence (`if (Roles & 
SymbolRole::NameReference) return true;`). I don't think the index size is 
concerning; clients don't have to store these occurrences if they don't care 
about them.

> Option (2): A client that wants to hardcode all navigation patterns in the 
> index can still do this -- in the `IndexDataConsumer`, it is possible to 
> handle the `ConstructorDecl` as a reference to the class, if desired.  The 
> flow of the indexing information becomes more clear in my opinion: when there 
> is a constructor decl in the source, the `IndexDataConsumer` gets one 
> `ConstructorDecl`. Then the specific implementation of `IndexDataConsumer` 
> decides to treat it also as a class reference, because it wants to support a 
> specific approach to performing symbol renaming.

I think the downside here is again the maintenance burden. This code would 
probably live downstream somewhere so whenever changes happen upstream that 
affect the code deriving these extra occurrences in clients that want it, or 
introduce new types of locations that those clients don't handle and miss, 
there's more duplicated effort updating/hunting down bugs. I also personally 
see these as legitimate occurrences of the type rather than hardcoded 
navigation patterns (see below), even though rename was the motivation here.

> As is, the indexing information is misleading and surprising.  When we see a 
> constructor decl or a constructor call, there is no reference to the class at 
> that point -- there is a reference to a constructor.  I think index is 
> supposed to expose semantic information, not just that there's a token in the 
> source code that has the same spelling as the class name.
> 
>> could we instead distinguish these cases with a more specific SymbolRole, 
>> e.g. NameReference as opposed to Reference, and filter them based on that 
>> instead?
> 
> It feels like a workaround to me, that also pushes the fix to the clients of 
> the indexing API. The core of the problem still exists: the indexing 
> information is surprising, and requires extra work on the client to make it 
> not surprising (filtering out NameReferences).

I agree that most people think of it as purely being the constructor name, but 
I don't think the semantic connection to the class is really as loose as 'a 
token with the same spelling' and I think it's reasonable to report it. To 
resolve a constructor call, the compiler looks for a *type* with that spelling. 
It's not as if the user chooses to name their constructor with the same name as 
the type – the name lookup finds the type. One interesting consequence of this 
is that the compiler seems to complain if you try to reference the constructor 
as opposed to the type when you call it, e.g.

  class A {
  public:
A(int x) {}
  };
  
  int main() {
auto x = A(2); // ok
auto y = A::A(2); // error: qualified reference to 'A' is a constructor 

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Nathan Hawes via Phabricator via cfe-commits]

nathawes requested changes to this revision.
nathawes added a comment.
This revision now requires changes to proceed.

These references were added to support using the index data for symbol rename. 
I.e. so that when you rename the class, you can use the index data to find all 
occurrences of its name, including its use in constructor/destructor 
declarations and references (hence the test cases in 
test/Index/Core/index-source.cpp). If you need to specifically find references 
of the class symbol, as opposed to its name, could we instead distinguish these 
cases with a more specific SymbolRole, e.g. NameReference as opposed to 
Reference, and filter them based on that instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Nathan Hawes via Phabricator via cfe-commits]

nathawes accepted this revision.
nathawes added a comment.
This revision is now accepted and ready to land.

Looks good to me.


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

https://reviews.llvm.org/D58478



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 153190.
nathawes added a comment.

Updated to apply on top-of-tree.


https://reviews.llvm.org/D39050

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticIndexKinds.td
  include/clang/Driver/Job.h
  include/clang/Driver/Options.td
  include/clang/Frontend/CompilerInstance.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Index/DeclOccurrence.h
  include/clang/Index/IndexDataConsumer.h
  include/clang/Index/IndexDiagnostic.h
  include/clang/Index/IndexingAction.h
  include/clang/Index/RecordingAction.h
  include/clang/Index/UnitIndexDataConsumer.h
  include/clang/Index/UnitIndexingAction.h
  include/clang/module.modulemap
  lib/Basic/DiagnosticIDs.cpp
  lib/Driver/Driver.cpp
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Darwin.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/CMakeLists.txt
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Index/CMakeLists.txt
  lib/Index/FileIndexData.cpp
  lib/Index/FileIndexData.h
  lib/Index/IndexingAction.cpp
  lib/Index/IndexingContext.cpp
  lib/Index/IndexingContext.h
  lib/Index/UnitIndexDataRecorder.cpp
  lib/Index/UnitIndexDataRecorder.h
  test/Index/Core/Inputs/module/ModDep.h
  test/Index/Core/Inputs/module/ModSystem.h
  test/Index/Core/Inputs/module/ModTop.h
  test/Index/Core/Inputs/module/ModTopSub1.h
  test/Index/Core/Inputs/module/ModTopSub2.h
  test/Index/Core/Inputs/module/module.modulemap
  test/Index/Core/Inputs/sys/system-head.h
  test/Index/Core/Inputs/transitive-include.h
  test/Index/Core/external-source-symbol-attr.m
  test/Index/Core/index-instantiated-source.cpp
  test/Index/Core/index-source.mm
  test/Index/Core/index-subkinds.m
  test/Index/Core/index-system.mm
  test/Index/Core/index-unit.mm
  test/Index/Store/assembly-invocation.c
  tools/c-index-test/core_main.cpp
  tools/diagtool/DiagnosticNames.cpp
  tools/libclang/CXIndexDataConsumer.cpp
  tools/libclang/CXIndexDataConsumer.h

Index: tools/libclang/CXIndexDataConsumer.h
===
--- tools/libclang/CXIndexDataConsumer.h
+++ tools/libclang/CXIndexDataConsumer.h
@@ -465,11 +465,12 @@
 private:
   bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations,
-   SourceLocation Loc, ASTNodeInfo ASTNode) override;
+   SourceLocation Loc, bool IsInSystemFile,
+   ASTNodeInfo ASTNode) override;
 
   bool handleModuleOccurence(const ImportDecl *ImportD,
- index::SymbolRoleSet Roles,
- SourceLocation Loc) override;
+ index::SymbolRoleSet Roles, SourceLocation Loc,
+ bool IsInSystemFile) override;
 
   void finish() override;
 
Index: tools/libclang/CXIndexDataConsumer.cpp
===
--- tools/libclang/CXIndexDataConsumer.cpp
+++ tools/libclang/CXIndexDataConsumer.cpp
@@ -157,7 +157,7 @@
 
 bool CXIndexDataConsumer::handleDeclOccurence(
 const Decl *D, SymbolRoleSet Roles, ArrayRef Relations,
-SourceLocation Loc, ASTNodeInfo ASTNode) {
+SourceLocation Loc, bool IsInSystemFile, ASTNodeInfo ASTNode) {
   Loc = getASTContext().getSourceManager().getFileLoc(Loc);
 
   if (Roles & (unsigned)SymbolRole::Reference) {
@@ -223,7 +223,8 @@
 
 bool CXIndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
 SymbolRoleSet Roles,
-SourceLocation Loc) {
+SourceLocation Loc,
+bool IsInSystemFile) {
   IndexingDeclVisitor(*this, SourceLocation(), nullptr).Visit(ImportD);
   return !shouldAbort();
 }
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -43,6 +43,7 @@
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
 #include "clang/Basic/DiagnosticRefactoringKinds.inc"
+#include "clang/Basic/DiagnosticIndexKinds.inc"
 #undef DIAG
 };
 
Index: tools/c-index-test/core_main.cpp
===
--- tools/c-index-test/core_main.cpp
+++ tools/c-index-test/core_main.cpp
@@ -12,15 +12,17 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
-#include "clang/Index/IndexingAction.h"
+#include "clang/Index/CodegenNameGenerator.h"
 

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added a comment.

In https://reviews.llvm.org/D39050#1037008, @malaperle wrote:

> In https://reviews.llvm.org/D39050#1036394, @nathawes wrote:
>
> > @malaperle Just to clarify, what's the particular end-loc we're talking 
> > about here? e.g. for a function call, would this be the end of the 
> > function's name, or the closing paren? 
> >  For the end of the name, couldn't this be derived from the start loc + 
> > symbol name length (barring token pastes and escaped new lines in the 
> > middle of identifiers, which hopefully aren't too common)?
> >  I can see the value for the closing paren though.
>
>
> I mean the end of the name referencing the symbol, so that it can be 
> highlighted properly when using the "find references in workspace" feature. 
> There are cases where the name of the symbol itself is not present, for 
> example "MyClass o1, o2;" (o1 and o2 reference the constructor), references 
> to overloaded operators, etc.


Ah, I see – thanks! I was thinking all occurrences whose symbol name didn't 
actually appear at their location were marked with `SymbolRole::Implicit`, but 
that only seems to be true for the ObjC index data.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added a comment.

In https://reviews.llvm.org/D39050#1036249, @malaperle wrote:

> In https://reviews.llvm.org/D39050#1021204, @malaperle wrote:
>
> > For computing the start/end-loc of function bodies, I tried the 
> > SingleFileParseMode and SkipFunctionBodies separately ( as a start). The 
> > source I use this on looks like this:
> >
> >  
>
>
> Given the discussion in https://reviews.llvm.org/D44247, I think we can do 
> without the start/end-loc of function bodies and try some heuristics 
> client-side. We can always revisit this later if necessary.
>
> However, for the end-loc of occurrences, would you be OK with this being 
> added? I think it would be a good compromise in terms of performance, 
> simplicity and index size.


@malaperle Just to clarify, what's the particular end-loc we're talking about 
here? e.g. for a function call, would this be the end of the function's name, 
or the closing paren? 
For the end of the name, couldn't this be derived from the start loc + symbol 
name length (barring token pastes and escaped new lines in the middle of 
identifiers, which hopefully aren't too common)?
I can see the value for the closing paren though.

@akyrtzi Are the numbers from Marc-Andre's experiment what you'd expect to see 
and is there anything else to try? I'm not familiar with those modes at all to 
comment, sorry. I assume any API to gather syntactic structure info would be 
based on those modes, right?


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added a comment.

@ioeric I'm working on a few other priorities over the next few weeks, sorry, 
but should get back to this relatively soon after that.
I would just land it, but I expect some downstream breakage I want to make sure 
I have time to fix.

@malaperle Sounds good – I'll keep an eye out for it!


https://reviews.llvm.org/D39050



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

[PATCH] D42588: [index] Fix crash when indexing a C++14 PCH/module related to TemplateTemplateParmDecls of alias templates

[Nathan Hawes via Phabricator via cfe-commits]

nathawes accepted this revision.
nathawes added a comment.
This revision is now accepted and ready to land.

Looks good to me!


Repository:
  rC Clang

https://reviews.llvm.org/D42588



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 130496.
nathawes marked 6 inline comments as done.
nathawes added a comment.

- Applied the various refactorings suggested by @ioeric
- Extended c-index-test with a new option to print out the collected unit 
indexing data, and
- Added tests for the unit indexing functionality using the new option
- Fixed formatting


https://reviews.llvm.org/D39050

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticIndexKinds.td
  include/clang/Driver/Job.h
  include/clang/Driver/Options.td
  include/clang/Frontend/CompilerInstance.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Index/DeclOccurrence.h
  include/clang/Index/IndexDataConsumer.h
  include/clang/Index/IndexDiagnostic.h
  include/clang/Index/IndexingAction.h
  include/clang/Index/RecordingAction.h
  include/clang/Index/UnitIndexDataConsumer.h
  include/clang/Index/UnitIndexingAction.h
  include/clang/module.modulemap
  lib/Basic/DiagnosticIDs.cpp
  lib/Driver/Driver.cpp
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Darwin.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/CMakeLists.txt
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Index/CMakeLists.txt
  lib/Index/FileIndexData.cpp
  lib/Index/FileIndexData.h
  lib/Index/IndexingAction.cpp
  lib/Index/IndexingContext.cpp
  lib/Index/IndexingContext.h
  lib/Index/UnitIndexDataRecorder.cpp
  lib/Index/UnitIndexDataRecorder.h
  test/Index/Core/Inputs/module/ModDep.h
  test/Index/Core/Inputs/module/ModSystem.h
  test/Index/Core/Inputs/module/ModTop.h
  test/Index/Core/Inputs/module/ModTopSub1.h
  test/Index/Core/Inputs/module/ModTopSub2.h
  test/Index/Core/Inputs/module/module.modulemap
  test/Index/Core/Inputs/sys/system-head.h
  test/Index/Core/Inputs/transitive-include.h
  test/Index/Core/external-source-symbol-attr.m
  test/Index/Core/index-instantiated-source.cpp
  test/Index/Core/index-source.mm
  test/Index/Core/index-subkinds.m
  test/Index/Core/index-system.mm
  test/Index/Core/index-unit.mm
  test/Index/Store/assembly-invocation.c
  tools/c-index-test/core_main.cpp
  tools/diagtool/DiagnosticNames.cpp
  tools/libclang/CXIndexDataConsumer.cpp
  tools/libclang/CXIndexDataConsumer.h

Index: tools/libclang/CXIndexDataConsumer.h
===
--- tools/libclang/CXIndexDataConsumer.h
+++ tools/libclang/CXIndexDataConsumer.h
@@ -463,12 +463,12 @@
 private:
   bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   FileID FID, unsigned Offset, bool IsInSystemFile,
ASTNodeInfo ASTNode) override;
 
   bool handleModuleOccurence(const ImportDecl *ImportD,
- index::SymbolRoleSet Roles,
- FileID FID, unsigned Offset) override;
+ index::SymbolRoleSet Roles, FileID FID,
+ unsigned Offset, bool IsInSystemFile) override;
 
   void finish() override;
 
Index: tools/libclang/CXIndexDataConsumer.cpp
===
--- tools/libclang/CXIndexDataConsumer.cpp
+++ tools/libclang/CXIndexDataConsumer.cpp
@@ -150,11 +150,9 @@
 };
 }
 
-bool CXIndexDataConsumer::handleDeclOccurence(const Decl *D,
-  SymbolRoleSet Roles,
- ArrayRef Relations,
-  FileID FID, unsigned Offset,
-  ASTNodeInfo ASTNode) {
+bool CXIndexDataConsumer::handleDeclOccurence(
+const Decl *D, SymbolRoleSet Roles, ArrayRef Relations,
+FileID FID, unsigned Offset, bool IsInSystemFile, ASTNodeInfo ASTNode) {
   SourceLocation Loc = getASTContext().getSourceManager()
   .getLocForStartOfFile(FID).getLocWithOffset(Offset);
 
@@ -219,9 +217,9 @@
 }
 
 bool CXIndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
-SymbolRoleSet Roles,
-FileID FID,
-unsigned Offset) {
+SymbolRoleSet Roles, FileID FID,
+unsigned Offset,
+bool IsInSystemFile) {
   IndexingDeclVisitor(*this, SourceLocation(), nullptr).Visit(ImportD);
   return !shouldAbort();
 }
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes planned changes to this revision.
nathawes marked 32 inline comments as done.
nathawes added a comment.

@ioeric I should have an updated patch up shortly with your inline comments 
addressed + new tests. Thanks again for reviewing!




Comment at: include/clang/Index/IndexUnitDataConsumer.h:67
+private:
+  virtual void _anchor();
+};

ioeric wrote:
> Comment? Why do we actually need this?
From [[ 
https://stackoverflow.com/questions/34913197/what-is-vtable-anchoring-and-how-does-it-work-in-a-shared-object
 | here ]], my understanding is that it's an optimization to avoid the vtable 
being included in multiple translation units. I'm not sure if that's actually a 
problem, I was just following IndexDataConsumer's lead. Added a comment.



Comment at: include/clang/Index/IndexingAction.h:114
+std::unique_ptr
+createUnitIndexingAction(const UnitIndexingOptions ,
+ IndexUnitDataConsumerFactory ConsumerFactory,

ioeric wrote:
>  What is the intended user of this function? It's unclear how users could 
> obtain a `ConsumerFactory ` (i.e. `UnitDetails`) without the functionalities 
> in `UnitDataConsumerActionImpl `.
> 
> (Also see comment in the implementation of `createIndexDataRecordingAction`.)
Sorry, I'm not sure what you mean here. Users shouldn't need to know anything 
about `UnitDataConsumerActionImpl`, they just need to provide a lambda/function 
reference that takes a `CompilerInstance&` and a `UnitDetails` and returns an 
`IndexUnitDataConsumer` (or `UnitIndexDataConsumer` once I rename it). This 
gets called once per translation unit to get a distinct data consumer for each 
unit, i.e. for the main translation unit as well as for each of its dependent 
modules that the main unit's data consumer says should be indexed via 
`shouldIndexModuleDependency(...)`.



Comment at: include/clang/Index/IndexingAction.h:128
+RecordingOptions
+getRecordingOptionsFromFrontendOptions(const FrontendOptions );
+

ioeric wrote:
> This is likely only useful for compiler invocation. I would put it in the 
> compiler invocation code.
There's another public `index::` API for writing out index data for individual 
clang module files in the follow up patch that takes a `RecordingOptions` and 
is used externally, from Swift. This function's useful on the Swift side to get 
the `RecordingOptions` from `FrontendOptions` it has already set up.



Comment at: lib/Index/IndexingAction.cpp:137
+
+CreatedASTConsumer = true;
+std::vector Consumers;

ioeric wrote:
> nit: Move this after `Impl->createIndexASTConsumer(CI)`.
> 
> Do we need to reset this flag? Calling `CreateASTConsumer ` multiple times on 
> the same instance seems to be allowed? 
Oops. Yes, we do :-)



Comment at: lib/Index/IndexingAction.cpp:504
+  /// \returns true if \c Mod was indexed
+  static bool indexModule(
+  const CompilerInstance , serialization::ModuleFile ,

ioeric wrote:
> Non-factory static method is often a code smell. Any reason not to make these 
> static methods private members? With that, you wouldn't need to pass along so 
> many parameters. You could make them `const` if you don't want members to be 
> modified.
Sorry, there's missing context – they're used from another public API that's in 
the follow-up patch. I'll bring that over and make these top-level static 
functions, since they don't belong exclusively to IndexDataConsumerActionImpl.



Comment at: lib/Index/IndexingAction.cpp:511
+  /// Get unit details for the given module file
+  static UnitDetails getUnitDetails(serialization::ModuleFile ,
+const CompilerInstance ,

ioeric wrote:
> Why is this overload public while others are private? Aren't they all used 
> only in this class?
Same as above – this is called from a public `index::` API in the follow-up 
patch.



Comment at: lib/Index/IndexingAction.cpp:758
+
+  class IndexUnitDataRecorder : public IndexUnitDataConsumer {
+  public:

ioeric wrote:
> I think the inheritance of `IndexUnitDataConsumer`  and the creation of 
> factory should be in user code (e.g. implementation for on-disk 
> persist-index-data should come from the compiler invocation code 
> `ExecuteCompilerInvocation.cpp` or at least a separate file in the library 
> that compiler invocation can use), and the user should only use 
> `createUnitIndexingAction` by providing a factory.  Currently, 
> `createUnitIndexingAction` and `createIndexDataRecordingAction` are mostly 
> identical except for the code that implements `IndexUnitDataConsumer ` and 
> creates the factory.
> 
> The current `createIndexDataRecordingAction` would probably only used by the 
> compiler invocation, and we can keep the generalized 
> `createUnitIndexingAction` in the public 

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 127568.
nathawes added a comment.

Fix out of data header comment in FileIndexData.h


https://reviews.llvm.org/D39050

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticIndexKinds.td
  include/clang/Driver/Job.h
  include/clang/Driver/Options.td
  include/clang/Frontend/CompilerInstance.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Index/DeclOccurrence.h
  include/clang/Index/IndexDataConsumer.h
  include/clang/Index/IndexDiagnostic.h
  include/clang/Index/IndexUnitDataConsumer.h
  include/clang/Index/IndexingAction.h
  include/clang/module.modulemap
  lib/Basic/DiagnosticIDs.cpp
  lib/Driver/Driver.cpp
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Darwin.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/CMakeLists.txt
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Index/CMakeLists.txt
  lib/Index/FileIndexData.cpp
  lib/Index/FileIndexData.h
  lib/Index/IndexingAction.cpp
  lib/Index/IndexingContext.cpp
  lib/Index/IndexingContext.h
  test/Index/Store/assembly-invocation.c
  tools/c-index-test/core_main.cpp
  tools/diagtool/DiagnosticNames.cpp
  tools/libclang/CXIndexDataConsumer.cpp
  tools/libclang/CXIndexDataConsumer.h

Index: tools/libclang/CXIndexDataConsumer.h
===
--- tools/libclang/CXIndexDataConsumer.h
+++ tools/libclang/CXIndexDataConsumer.h
@@ -463,12 +463,12 @@
 private:
   bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   FileID FID, unsigned Offset, bool IsInSystemFile,
ASTNodeInfo ASTNode) override;
 
   bool handleModuleOccurence(const ImportDecl *ImportD,
- index::SymbolRoleSet Roles,
- FileID FID, unsigned Offset) override;
+ index::SymbolRoleSet Roles, FileID FID,
+ unsigned Offset, bool IsInSystemFile) override;
 
   void finish() override;
 
Index: tools/libclang/CXIndexDataConsumer.cpp
===
--- tools/libclang/CXIndexDataConsumer.cpp
+++ tools/libclang/CXIndexDataConsumer.cpp
@@ -150,11 +150,9 @@
 };
 }
 
-bool CXIndexDataConsumer::handleDeclOccurence(const Decl *D,
-  SymbolRoleSet Roles,
- ArrayRef Relations,
-  FileID FID, unsigned Offset,
-  ASTNodeInfo ASTNode) {
+bool CXIndexDataConsumer::handleDeclOccurence(
+const Decl *D, SymbolRoleSet Roles, ArrayRef Relations,
+FileID FID, unsigned Offset, bool IsInSystemFile, ASTNodeInfo ASTNode) {
   SourceLocation Loc = getASTContext().getSourceManager()
   .getLocForStartOfFile(FID).getLocWithOffset(Offset);
 
@@ -219,9 +217,9 @@
 }
 
 bool CXIndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
-SymbolRoleSet Roles,
-FileID FID,
-unsigned Offset) {
+SymbolRoleSet Roles, FileID FID,
+unsigned Offset,
+bool IsInSystemFile) {
   IndexingDeclVisitor(*this, SourceLocation(), nullptr).Visit(ImportD);
   return !shouldAbort();
 }
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -43,6 +43,7 @@
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
 #include "clang/Basic/DiagnosticRefactoringKinds.inc"
+#include "clang/Basic/DiagnosticIndexKinds.inc"
 #undef DIAG
 };
 
Index: tools/c-index-test/core_main.cpp
===
--- tools/c-index-test/core_main.cpp
+++ tools/c-index-test/core_main.cpp
@@ -87,8 +87,8 @@
   }
 
   bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
-   ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   ArrayRef Relations, FileID FID,
+   unsigned Offset, bool IsInSystemFile,
ASTNodeInfo ASTNode) override {
 ASTContext  = D->getASTContext();
 SourceManager  = Ctx.getSourceManager();
@@ -124,7 +124,8 @@
   }
 
   bool 

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 127412.
nathawes marked an inline comment as done.
nathawes added a comment.

I've refactored the indexing/dependency data collection out from the writing 
with the new IndexUnitDataConsumer class, and made other smaller changes to 
address the feedback from @ioeric.


https://reviews.llvm.org/D39050

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticIndexKinds.td
  include/clang/Driver/Job.h
  include/clang/Driver/Options.td
  include/clang/Frontend/CompilerInstance.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Index/DeclOccurrence.h
  include/clang/Index/IndexDataConsumer.h
  include/clang/Index/IndexDiagnostic.h
  include/clang/Index/IndexUnitDataConsumer.h
  include/clang/Index/IndexingAction.h
  include/clang/module.modulemap
  lib/Basic/DiagnosticIDs.cpp
  lib/Driver/Driver.cpp
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Darwin.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/CMakeLists.txt
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Index/CMakeLists.txt
  lib/Index/FileIndexData.cpp
  lib/Index/FileIndexData.h
  lib/Index/IndexingAction.cpp
  lib/Index/IndexingContext.cpp
  lib/Index/IndexingContext.h
  test/Index/Store/assembly-invocation.c
  tools/c-index-test/core_main.cpp
  tools/diagtool/DiagnosticNames.cpp
  tools/libclang/CXIndexDataConsumer.cpp
  tools/libclang/CXIndexDataConsumer.h

Index: tools/libclang/CXIndexDataConsumer.h
===
--- tools/libclang/CXIndexDataConsumer.h
+++ tools/libclang/CXIndexDataConsumer.h
@@ -463,12 +463,12 @@
 private:
   bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   FileID FID, unsigned Offset, bool IsInSystemFile,
ASTNodeInfo ASTNode) override;
 
   bool handleModuleOccurence(const ImportDecl *ImportD,
- index::SymbolRoleSet Roles,
- FileID FID, unsigned Offset) override;
+ index::SymbolRoleSet Roles, FileID FID,
+ unsigned Offset, bool IsInSystemFile) override;
 
   void finish() override;
 
Index: tools/libclang/CXIndexDataConsumer.cpp
===
--- tools/libclang/CXIndexDataConsumer.cpp
+++ tools/libclang/CXIndexDataConsumer.cpp
@@ -150,11 +150,9 @@
 };
 }
 
-bool CXIndexDataConsumer::handleDeclOccurence(const Decl *D,
-  SymbolRoleSet Roles,
- ArrayRef Relations,
-  FileID FID, unsigned Offset,
-  ASTNodeInfo ASTNode) {
+bool CXIndexDataConsumer::handleDeclOccurence(
+const Decl *D, SymbolRoleSet Roles, ArrayRef Relations,
+FileID FID, unsigned Offset, bool IsInSystemFile, ASTNodeInfo ASTNode) {
   SourceLocation Loc = getASTContext().getSourceManager()
   .getLocForStartOfFile(FID).getLocWithOffset(Offset);
 
@@ -219,9 +217,9 @@
 }
 
 bool CXIndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
-SymbolRoleSet Roles,
-FileID FID,
-unsigned Offset) {
+SymbolRoleSet Roles, FileID FID,
+unsigned Offset,
+bool IsInSystemFile) {
   IndexingDeclVisitor(*this, SourceLocation(), nullptr).Visit(ImportD);
   return !shouldAbort();
 }
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -43,6 +43,7 @@
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
 #include "clang/Basic/DiagnosticRefactoringKinds.inc"
+#include "clang/Basic/DiagnosticIndexKinds.inc"
 #undef DIAG
 };
 
Index: tools/c-index-test/core_main.cpp
===
--- tools/c-index-test/core_main.cpp
+++ tools/c-index-test/core_main.cpp
@@ -87,8 +87,8 @@
   }
 
   bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
-   ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   ArrayRef Relations, FileID FID,
+   unsigned Offset, bool IsInSystemFile,
 

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes marked 45 inline comments as done.
nathawes added inline comments.



Comment at: lib/Index/FileIndexRecord.h:51
+public:
+  FileIndexRecord(FileID FID, bool isSystem) : FID(FID), IsSystem(isSystem) {}
+

ioeric wrote:
> s/isSystem/IsSystem/
> 
> Also, I wonder if we can filter out system decls proactively and avoid 
> creating file index record for them. We could also avoid propogating 
> `IsSystem` here.
If the -index-ignore-system-symbols flag is set system decls are filtered out 
in IndexingContext::handleDeclOccurrence and aren't reported to the 
IndexDataConsumer, so FileIndexRecords won't be created. The IsSystem here is 
for clients that want index data for system files, but want to be able to 
distinguish them from regular files.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes planned changes to this revision.
nathawes added a comment.

Thanks for taking another look @ioeric – I'll work through your comments and 
update.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes updated this revision to Diff 126065.
nathawes added a comment.
Herald added a subscriber: mgrang.

Worked through the comments from @ioeric and split the code for writing out the 
collected indexing data into a separate patch.


https://reviews.llvm.org/D39050

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticIndexKinds.td
  include/clang/Driver/Job.h
  include/clang/Driver/Options.td
  include/clang/Frontend/CompilerInstance.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Index/IndexDataConsumer.h
  include/clang/Index/IndexDiagnostic.h
  include/clang/Index/IndexingAction.h
  include/clang/module.modulemap
  lib/Basic/DiagnosticIDs.cpp
  lib/Driver/Driver.cpp
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Darwin.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/CMakeLists.txt
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Index/CMakeLists.txt
  lib/Index/FileIndexRecord.cpp
  lib/Index/FileIndexRecord.h
  lib/Index/IndexingAction.cpp
  lib/Index/IndexingContext.cpp
  lib/Index/IndexingContext.h
  test/Index/Store/assembly-invocation.c
  tools/c-index-test/core_main.cpp
  tools/diagtool/DiagnosticNames.cpp
  tools/libclang/CXIndexDataConsumer.cpp
  tools/libclang/CXIndexDataConsumer.h

Index: tools/libclang/CXIndexDataConsumer.h
===
--- tools/libclang/CXIndexDataConsumer.h
+++ tools/libclang/CXIndexDataConsumer.h
@@ -463,12 +463,12 @@
 private:
   bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   FileID FID, unsigned Offset, bool IsInSystemFile,
ASTNodeInfo ASTNode) override;
 
   bool handleModuleOccurence(const ImportDecl *ImportD,
- index::SymbolRoleSet Roles,
- FileID FID, unsigned Offset) override;
+ index::SymbolRoleSet Roles, FileID FID,
+ unsigned Offset, bool IsInSystemFile) override;
 
   void finish() override;
 
Index: tools/libclang/CXIndexDataConsumer.cpp
===
--- tools/libclang/CXIndexDataConsumer.cpp
+++ tools/libclang/CXIndexDataConsumer.cpp
@@ -150,11 +150,9 @@
 };
 }
 
-bool CXIndexDataConsumer::handleDeclOccurence(const Decl *D,
-  SymbolRoleSet Roles,
- ArrayRef Relations,
-  FileID FID, unsigned Offset,
-  ASTNodeInfo ASTNode) {
+bool CXIndexDataConsumer::handleDeclOccurence(
+const Decl *D, SymbolRoleSet Roles, ArrayRef Relations,
+FileID FID, unsigned Offset, bool IsInSystemFile, ASTNodeInfo ASTNode) {
   SourceLocation Loc = getASTContext().getSourceManager()
   .getLocForStartOfFile(FID).getLocWithOffset(Offset);
 
@@ -219,9 +217,9 @@
 }
 
 bool CXIndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
-SymbolRoleSet Roles,
-FileID FID,
-unsigned Offset) {
+SymbolRoleSet Roles, FileID FID,
+unsigned Offset,
+bool IsInSystemFile) {
   IndexingDeclVisitor(*this, SourceLocation(), nullptr).Visit(ImportD);
   return !shouldAbort();
 }
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -43,6 +43,7 @@
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
 #include "clang/Basic/DiagnosticRefactoringKinds.inc"
+#include "clang/Basic/DiagnosticIndexKinds.inc"
 #undef DIAG
 };
 
Index: tools/c-index-test/core_main.cpp
===
--- tools/c-index-test/core_main.cpp
+++ tools/c-index-test/core_main.cpp
@@ -87,8 +87,8 @@
   }
 
   bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
-   ArrayRef Relations,
-   FileID FID, unsigned Offset,
+   ArrayRef Relations, FileID FID,
+   unsigned Offset, bool IsInSystemFile,
ASTNodeInfo ASTNode) override {
 ASTContext  = D->getASTContext();
 SourceManager  = Ctx.getSourceManager();
@@ -124,7 +124,8 

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added inline comments.



Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:170
+Act = index::createIndexDataRecordingAction(FEOpts, std::move(Act));
+CI.setGenModuleActionWrapper(::createIndexDataRecordingAction);
+  }

ioeric wrote:
> Could you comment on what this does? The `Act` above is already wrapped. Why 
> do we need `setGenModuleActionWrapper` to `createIndexDataRecordingAction` 
> again? Also, `createIndexDataRecordingAction` doesn't seem related to 
> `GenModule`.
It's to wrap any GenerateModuleActions that get created as needed when/if Act 
ends up loading any modules, so that we output index data for them too. I'll 
add a comment.



Comment at: lib/Index/FileIndexRecord.cpp:39
+  auto It = std::upper_bound(Decls.begin(), Decls.end(), NewInfo);
+  Decls.insert(It, std::move(NewInfo));
+}

ioeric wrote:
> Why do we need `Decls` to be sorted by offset? If we want this for printing, 
> it might make sense to just do a sort there.
It's mostly for when we hash them, so that ordering doesn't change the hash, 
but it's also for printing. The IndexASTConsumer doesn't always report symbol 
occurrences in source order, due to the preprocessor and a few other cases.
We can sort them when the IndexRecordDataConsumer's finish() is called rather 
than as they're added to avoid the copying from repeated insert calls if that's 
the concern.



Comment at: lib/Index/IndexingAction.cpp:504
+
+CreatedASTConsumer = true;
+std::vector Consumers;

ioeric wrote:
>  Can we get this state from the base class instead of maintaining a another 
> state, which seems to be identical?
I don't see this state in either base class (WrapperFrontendAction and 
IndexRecordActionBase). WrappingIndexAction and WrappingIndexRecordAction both 
have this, though. Were you thinking a new intermediate common base class 
between them and WrapperFrontendAction?



Comment at: lib/Index/IndexingAction.cpp:769
+  IndexCtx.setSysrootPath(SysrootPath);
+  Recorder.init(, CI);
+

ioeric wrote:
> It's a bit worrying that `IndexDataRecorder` and `IndexContext` reference 
> each other. If you only need some information from the `IndexingContext`, 
> simply pass it into `Recorder`. In this case, I think you only need the 
> `SourceManager`  from the `ASTContext` in the recorder to calculate whether a 
> file is a system header. I see you also cache result of 
> `IndexingContext::isSystemFile` in the indexing context, but I think it would 
> be more sensible for the callers to handle caching for this call.
Good point. The IndexingContext was actually already calling IsSystemFile 
before it calls IndexDataRecorder's handleDeclOccurrence and 
handleModuleOccurrence anyway, so I'll change it to pass that through as an 
extra param and remove IndexDataRecorder's dependency on the IndexingContext.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes planned changes to this revision.
nathawes added a comment.

In https://reviews.llvm.org/D39050#920451, @ioeric wrote:

> In https://reviews.llvm.org/D39050#900830, @arphaman wrote:
>
> > I think this patch should be split into a number of smaller patches to help 
> > the review process.
> >
> > Things like `tools/IndexStore`, `DirectoryWatcher` and other components 
> > that are not directly needed right now should definitely be in their own 
> > patches.
> >  It would be nice to find some way to split the implementation into 
> > multiple patches as well.
>
>
> +1.
>
> This is a lot of work (but great work!) for one patch. Smaller/incremental 
> patches help reviewers understand and (hopefully) capture potential 
> improvement of the design. I would really appreciate it if you could further 
> split the patch.


Thanks for taking a look @ioeric! I'll have a go at splitting it further.

> Some comments/ideas:
> 
> - The lack of tests is a bit concerning.

I moved all the code for reading the index store data into a separate patch (to 
come after this one) in order to slim this one down for review, and most of the 
tests went with it because they're based around reading and dumping the stored 
data for FileCheck. The original version of this patch has them all 
(https://reviews.llvm.org/D39050?id=118854). The ones that remain here are just 
those checking that the unit/record files are written out and that the hashing 
mechanism is producing distinct record files when the symbolic content of the 
source file changes.

> - I think the implementation of the index output logic (e.g. 
> `IndexUnitWriter` and bit format file) can be abstracted away (and split into 
> separate patches) so that you can unit-test the action with a custom/mock 
> unit writer; this would also make the action reusable with (potentially) 
> other storage formats.

The added IndexRecordAction and existing IndexAction use the same functionality 
from libIndex to collect the indexing data, so I'm not sure mocking the unit 
writer to unit test IndexRecordAction would add very much value – writing the 
index data out is the new behavior. The existing tests for IndexAction (under 
test/Index/Core) are already covering the correctness of the majority of the 
collected indexing info and the tests coming in the follow-up patch (seen in 
the original version of this patch) test it's still correct after the 
write/read round trip.

> - I would suggest that you start with a patch that implement the index action 
> and just enough components so that you could test the action.
> 
>   Thanks!


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added inline comments.



Comment at: lib/Index/IndexRecordHasher.cpp:103
+  COMBINE_HASH(Hasher.hash(param->getType()));
+}
+return Hash;

arphaman wrote:
> Should you hash the return type as well?
The return type doesn't affect the function's USR, so there's no need to 
consider it when hashing the function decl. The hashing here is happening per 
decl occurrence (source offset + role set + Decl) collected by the index AST 
walker, so changing the return type will still change the record hash when any 
decl occurrences it contains are hashed in.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Nathan Hawes via Phabricator via cfe-commits]

nathawes planned changes to this revision.
nathawes added a comment.

Thanks @arphaman! I'll work through your comments and update.




Comment at: include/clang/Index/IndexDataStoreSymbolUtils.h:13
+
+#include "indexstore/indexstore.h"
+#include "clang/Index/IndexSymbol.h"

arphaman wrote:
> It looks to me like this header, `"indexstore/indexstore.h"`, and 
> `IndexDataStoreUtils.cpp` are utilities just for the C API, so could we take 
> it out of here as well?
They're used by IndexRecordWriter below to convert from libIndex's 
representation of things to the index store's.


https://reviews.llvm.org/D39050



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

[PATCH] D36641: [index] Update indexing to handle CXXDeductionGuideDecls properly

[Nathan Hawes via Phabricator via cfe-commits]

nathawes created this revision.

CXXDeductionGuideDecls can't be referenced so there's no need to output a 
symbol occurrence for them. Also handle DeducedTemplateSpecializationTypeLocs 
in the TypeIndexer so we don't miss the symbol occurrences of the corresponding 
template decls.


https://reviews.llvm.org/D36641

Files:
  lib/Index/IndexDecl.cpp
  lib/Index/IndexTypeSourceInfo.cpp
  lib/Index/IndexingContext.cpp
  test/Index/Core/index-source.cpp


Index: test/Index/Core/index-source.cpp
===
--- test/Index/Core/index-source.cpp
+++ test/Index/Core/index-source.cpp
@@ -497,6 +497,19 @@
 
 }
 
+template
+struct Guided { T t; };
+// CHECK: [[@LINE-1]]:8 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | 
 | Def | rel: 0
+// CHECK-NEXT: [[@LINE-2]]:19 | field/C++ | t | c:@ST>1#T@Guided@FI@t | 
 | Def,RelChild | rel: 1
+// CHECK-NEXT: RelChild | Guided | c:@ST>1#T@Guided
+Guided(double) -> Guided;
+// CHECK: [[@LINE-1]]:19 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | 
 | Ref | rel: 0
+// CHECK-NEXT: [[@LINE-2]]:1 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | 
 | Ref | rel: 0
+auto guided = Guided{1.0};
+// CHECK: [[@LINE-1]]:6 | variable/C | guided | c:@guided | _guided | Def | 
rel: 0
+// CHECK-NEXT: [[@LINE-2]]:15 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | 
 | Ref,RelCont | rel: 1
+// CHECK-NEXT: RelCont | guided | c:@guided
+
 namespace rd33122110 {
 
 struct Outer {
Index: lib/Index/IndexingContext.cpp
===
--- lib/Index/IndexingContext.cpp
+++ lib/Index/IndexingContext.cpp
@@ -231,8 +231,8 @@
 
 /// Whether the given NamedDecl should be skipped because it has no name.
 static bool shouldSkipNamelessDecl(const NamedDecl *ND) {
-  return ND->getDeclName().isEmpty() && !isa(ND) &&
- !isa(ND);
+  return (ND->getDeclName().isEmpty() && !isa(ND) &&
+  !isa(ND)) || isa(ND);
 }
 
 static const Decl *adjustParent(const Decl *Parent) {
Index: lib/Index/IndexTypeSourceInfo.cpp
===
--- lib/Index/IndexTypeSourceInfo.cpp
+++ lib/Index/IndexTypeSourceInfo.cpp
@@ -126,8 +126,9 @@
 return true;
   }
 
-  bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc TL) {
-if (const TemplateSpecializationType *T = TL.getTypePtr()) {
+  template
+  bool HandleTemplateSpecializationTypeLoc(TypeLocType TL) {
+if (const auto *T = TL.getTypePtr()) {
   if (IndexCtx.shouldIndexImplicitTemplateInsts()) {
 if (CXXRecordDecl *RD = T->getAsCXXRecordDecl())
   IndexCtx.handleReference(RD, TL.getTemplateNameLoc(),
@@ -141,6 +142,14 @@
 return true;
   }
 
+  bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc TL) {
+return HandleTemplateSpecializationTypeLoc(TL);
+  }
+
+  bool 
VisitDeducedTemplateSpecializationTypeLoc(DeducedTemplateSpecializationTypeLoc 
TL) {
+return HandleTemplateSpecializationTypeLoc(TL);
+  }
+
   bool VisitDependentNameTypeLoc(DependentNameTypeLoc TL) {
 const DependentNameType *DNT = TL.getTypePtr();
 const NestedNameSpecifier *NNS = DNT->getQualifier();
Index: lib/Index/IndexDecl.cpp
===
--- lib/Index/IndexDecl.cpp
+++ lib/Index/IndexDecl.cpp
@@ -267,6 +267,10 @@
  TypeNameInfo->getTypeLoc().getLocStart(),
  Dtor->getParent(), Dtor->getDeclContext());
   }
+} else if (const auto *Guide = dyn_cast(D)) {
+  IndexCtx.handleReference(Guide->getDeducedTemplate()->getTemplatedDecl(),
+   Guide->getLocation(), Guide,
+   Guide->getDeclContext());
 }
 // Template specialization arguments.
 if (const ASTTemplateArgumentListInfo *TemplateArgInfo =


Index: test/Index/Core/index-source.cpp
===
--- test/Index/Core/index-source.cpp
+++ test/Index/Core/index-source.cpp
@@ -497,6 +497,19 @@
 
 }
 
+template
+struct Guided { T t; };
+// CHECK: [[@LINE-1]]:8 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided |  | Def | rel: 0
+// CHECK-NEXT: [[@LINE-2]]:19 | field/C++ | t | c:@ST>1#T@Guided@FI@t |  | Def,RelChild | rel: 1
+// CHECK-NEXT: RelChild | Guided | c:@ST>1#T@Guided
+Guided(double) -> Guided;
+// CHECK: [[@LINE-1]]:19 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided |  | Ref | rel: 0
+// CHECK-NEXT: [[@LINE-2]]:1 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided |  | Ref | rel: 0
+auto guided = Guided{1.0};
+// CHECK: [[@LINE-1]]:6 | variable/C | guided | c:@guided | _guided | Def | rel: 0
+// CHECK-NEXT: [[@LINE-2]]:15 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided |  | Ref,RelCont | rel: 1
+// CHECK-NEXT: RelCont | guided | c:@guided
+
 namespace rd33122110 {
 
 struct Outer {
Index: lib/Index/IndexingContext.cpp

[PATCH] D31179: Objective-C categories should support attributes

[Nathan Hawes via Phabricator via cfe-commits]

nathawes accepted this revision.
nathawes added a comment.
This revision is now accepted and ready to land.

Thanks for this – looks good to me!


Repository:
  rL LLVM

https://reviews.llvm.org/D31179



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