[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

@bruno and @dexonsmith thank you for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG178ad93e3f1f: [clang][clangd] Use reverse header map lookup 
in suggestPathToFileForDiagnostics (authored by DmitryPolukhin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1834,7 +1834,7 @@
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-// FIXME: Support this search within frameworks and header maps.
+// FIXME: Support this search within frameworks.
 if (!SearchDirs[I].isNormalDir())
   continue;
 
@@ -1848,6 +1848,19 @@
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
 *IsSystem = false;
 
+  // Try resolving resulting filename via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (!SearchDirs[I].isHeaderMap())
+  continue;
 
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+StringRef SpelledFilename =
+SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+if (!SpelledFilename.empty()) {
+  Filename = SpelledFilename;
+  break;
+}
+  }
+  return path::convert_to_slash(Filename);
 }
Index: clang/lib/Lex/HeaderMap.cpp
===
--- clang/lib/Lex/HeaderMap.cpp
+++ clang/lib/Lex/HeaderMap.cpp
@@ -240,3 +240,32 @@
 return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (!ReverseMap.empty())
+return ReverseMap.lookup(DestPath);
+
+  const HMapHeader &Hdr = getHeader();
+  unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+  StringRef RetKey;
+  for (unsigned i = 0; i != NumBuckets; ++i) {
+HMapBucket B = getBucket(i);
+if (B.Key == HMAP_EmptyBucketKey)
+  continue;
+
+Optional Key = getString(B.Key);
+Optional Prefix = getString(B.Prefix);
+Optional Suffix = getString(B.Suffix);
+if (LLVM_LIKELY(Key && Prefix && Suffix)) {
+  SmallVector Buf;
+  Buf.append(Prefix->begin(), Prefix->end());
+  Buf.append(Suffix->begin(), Suffix->end());
+  StringRef Value(Buf.begin(), Buf.

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added inline comments.



Comment at: clang/lib/Lex/HeaderMap.cpp:265
+  }
+  return ReverseMap.lookup(DestPath);
+}

bruno wrote:
> How about something along the lines of:
> 
> ```
> ...
> StringRef Key;
> for (unsigned i = 0; i != NumBuckets; ++i) {
> HMapBucket B = getBucket(i);
> if (B.Key == HMAP_EmptyBucketKey)
>   continue;
> 
> Key = getString(B.Key);
> Optional Prefix = getString(B.Prefix);
> Optional Suffix = getString(B.Suffix);
> if (!Key.empty() && Prefix && Suffix) {
>   SmallVector Buf;
>   Buf.append(Prefix->begin(), Prefix->end());
>   Buf.append(Suffix->begin(), Suffix->end());
>   ReverseMap[StringRef(Buf.begin(), Buf.size())] = Key;
>   break;
> }
> }
> assert(!Key.empty() && "expected valid key");
> return Key;
> ```
> 
> While proposing this change I've noticed that it would keep looking for other 
> buckets even in face of a valid result, so I've added a `break`. Was that 
> intentional? 
Yes, keep iterating over all key-value pairs in the header map is important to 
add all elements to the map for subsequent lookups. If we stop on the current 
match, next lookup will not even try to populate the reverse lookup map 
(because the map is not empty) but not all elements are in the map due to early 
return from previous lookup, so subsequent lookup may not find the match even 
if it exists in the header map. We also cannot expect that DestPath is present 
in the given header map, so we cannot assert if it is missing. This function 
should return empty string as an indication that DestPath is not found.

Updated implementation with the review suggestion, please take another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 349199.
DmitryPolukhin added a comment.

Updated HeaderMapImpl::reverseLookupFilename according to the review comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1834,7 +1834,7 @@
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-// FIXME: Support this search within frameworks and header maps.
+// FIXME: Support this search within frameworks.
 if (!SearchDirs[I].isNormalDir())
   continue;
 
@@ -1848,6 +1848,19 @@
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
 *IsSystem = false;
 
+  // Try resolving resulting filename via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (!SearchDirs[I].isHeaderMap())
+  continue;
 
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+StringRef SpelledFilename =
+SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+if (!SpelledFilename.empty()) {
+  Filename = SpelledFilename;
+  break;
+}
+  }
+  return path::convert_to_slash(Filename);
 }
Index: clang/lib/Lex/HeaderMap.cpp
===
--- clang/lib/Lex/HeaderMap.cpp
+++ clang/lib/Lex/HeaderMap.cpp
@@ -240,3 +240,32 @@
 return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (!ReverseMap.empty())
+return ReverseMap.lookup(DestPath);
+
+  const HMapHeader &Hdr = getHeader();
+  unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+  StringRef RetKey;
+  for (unsigned i = 0; i != NumBuckets; ++i) {
+HMapBucket B = getBucket(i);
+if (B.Key == HMAP_EmptyBucketKey)
+  continue;
+
+Optional Key = getString(B.Key);
+Optional Prefix = getString(B.Prefix);
+Optional Suffix = getString(B.Suffix);
+if (LLVM_LIKELY(Key && Prefix && Suffix)) {
+  SmallVector Buf;
+  Buf.append(Prefix->begin(), Prefix->end());
+  Buf.append(Suffix->begin(), Suffix->end());
+  StringRef Value(Buf.begin(), Buf.size());
+  ReverseMap[Value] = *Key;
+
+  if (

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: clang/lib/Lex/HeaderMap.cpp:265
+  }
+  return ReverseMap.lookup(DestPath);
+}

How about something along the lines of:

```
...
StringRef Key;
for (unsigned i = 0; i != NumBuckets; ++i) {
HMapBucket B = getBucket(i);
if (B.Key == HMAP_EmptyBucketKey)
  continue;

Key = getString(B.Key);
Optional Prefix = getString(B.Prefix);
Optional Suffix = getString(B.Suffix);
if (!Key.empty() && Prefix && Suffix) {
  SmallVector Buf;
  Buf.append(Prefix->begin(), Prefix->end());
  Buf.append(Suffix->begin(), Suffix->end());
  ReverseMap[StringRef(Buf.begin(), Buf.size())] = Key;
  break;
}
}
assert(!Key.empty() && "expected valid key");
return Key;
```

While proposing this change I've noticed that it would keep looking for other 
buckets even in face of a valid result, so I've added a `break`. Was that 
intentional? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 348965.
DmitryPolukhin added a comment.

Rebase + try build on Windows again


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1834,7 +1834,7 @@
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-// FIXME: Support this search within frameworks and header maps.
+// FIXME: Support this search within frameworks.
 if (!SearchDirs[I].isNormalDir())
   continue;
 
@@ -1848,6 +1848,19 @@
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
 *IsSystem = false;
 
+  // Try resolving resulting filename via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (!SearchDirs[I].isHeaderMap())
+  continue;
 
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+StringRef SpelledFilename =
+SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+if (!SpelledFilename.empty()) {
+  Filename = SpelledFilename;
+  break;
+}
+  }
+  return path::convert_to_slash(Filename);
 }
Index: clang/lib/Lex/HeaderMap.cpp
===
--- clang/lib/Lex/HeaderMap.cpp
+++ clang/lib/Lex/HeaderMap.cpp
@@ -240,3 +240,27 @@
 return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (!ReverseMap.empty())
+return ReverseMap.lookup(DestPath);
+
+  const HMapHeader &Hdr = getHeader();
+  unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+  for (unsigned i = 0; i != NumBuckets; ++i) {
+HMapBucket B = getBucket(i);
+if (B.Key == HMAP_EmptyBucketKey)
+  continue;
+
+Optional Key = getString(B.Key);
+Optional Prefix = getString(B.Prefix);
+Optional Suffix = getString(B.Suffix);
+if (Key && Prefix && Suffix) {
+  SmallVector Buf;
+  Buf.append(Prefix->begin(), Prefix->end());
+  Buf.append(Suffix->begin(), Suffix->end());
+  ReverseMap[StringRef(Buf.begin(), Buf.size())] = *Key;
+}
+  }
+  return ReverseMap.lookup(DestPath);
+}
Index: clang/include/clang/Lex/HeaderMap.h
===

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added inline comments.



Comment at: clang/lib/Lex/HeaderMap.cpp:245
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (ReverseMap.empty()) {
+const HMapHeader &Hdr = getHeader();

bruno wrote:
> Please rewrite this to early return in case this isn't empty. 
Done, but it causes a bit of code duplication due to second lookup. Please let 
me know if you thought about rewriting it somehow else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 348900.
DmitryPolukhin marked 2 inline comments as done.
DmitryPolukhin added a comment.

Resolved comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1834,7 +1834,7 @@
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-// FIXME: Support this search within frameworks and header maps.
+// FIXME: Support this search within frameworks.
 if (!SearchDirs[I].isNormalDir())
   continue;
 
@@ -1848,6 +1848,19 @@
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
 *IsSystem = false;
 
+  // Try resolving resulting filename via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (!SearchDirs[I].isHeaderMap())
+  continue;
 
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+StringRef SpelledFilename =
+SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+if (!SpelledFilename.empty()) {
+  Filename = SpelledFilename;
+  break;
+}
+  }
+  return path::convert_to_slash(Filename);
 }
Index: clang/lib/Lex/HeaderMap.cpp
===
--- clang/lib/Lex/HeaderMap.cpp
+++ clang/lib/Lex/HeaderMap.cpp
@@ -240,3 +240,27 @@
 return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (!ReverseMap.empty())
+return ReverseMap.lookup(DestPath);
+
+  const HMapHeader &Hdr = getHeader();
+  unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+  for (unsigned i = 0; i != NumBuckets; ++i) {
+HMapBucket B = getBucket(i);
+if (B.Key == HMAP_EmptyBucketKey)
+  continue;
+
+Optional Key = getString(B.Key);
+Optional Prefix = getString(B.Prefix);
+Optional Suffix = getString(B.Suffix);
+if (Key && Prefix && Suffix) {
+  SmallVector Buf;
+  Buf.append(Prefix->begin(), Prefix->end());
+  Buf.append(Suffix->begin(), Suffix->end());
+  ReverseMap[StringRef(Buf.begin(), Buf.size())] = *Key;
+}
+  }
+  return ReverseMap.lookup(DestPath);
+}
Index: clang/include/cl

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Overall looks good, few remaining nitpicks.




Comment at: clang/lib/Lex/HeaderMap.cpp:245
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (ReverseMap.empty()) {
+const HMapHeader &Hdr = getHeader();

Please rewrite this to early return in case this isn't empty. 



Comment at: clang/lib/Lex/HeaderSearch.cpp:1856
+if (SearchDirs[I].isHeaderMap()) {
+  StringRef SpelledFilename =
+  SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);

Similar from above: rewrite to early continue if it's not meeting the 
conditions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 348264.
DmitryPolukhin added a comment.

Fix forgotten comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1834,7 +1834,7 @@
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-// FIXME: Support this search within frameworks and header maps.
+// FIXME: Support this search within frameworks.
 if (!SearchDirs[I].isNormalDir())
   continue;
 
@@ -1848,6 +1848,18 @@
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
 *IsSystem = false;
 
-
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+  // Try resolving resulting filename via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (SearchDirs[I].isHeaderMap()) {
+  StringRef SpelledFilename =
+  SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+  if (!SpelledFilename.empty()) {
+Filename = SpelledFilename;
+break;
+  }
+}
+  }
+  return path::convert_to_slash(Filename);
 }
Index: clang/lib/Lex/HeaderMap.cpp
===
--- clang/lib/Lex/HeaderMap.cpp
+++ clang/lib/Lex/HeaderMap.cpp
@@ -240,3 +240,26 @@
 return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (ReverseMap.empty()) {
+const HMapHeader &Hdr = getHeader();
+unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+for (unsigned i = 0; i != NumBuckets; ++i) {
+  HMapBucket B = getBucket(i);
+  if (B.Key == HMAP_EmptyBucketKey)
+continue;
+
+  Optional Key = getString(B.Key);
+  Optional Prefix = getString(B.Prefix);
+  Optional Suffix = getString(B.Suffix);
+  if (Key && Prefix && Suffix) {
+SmallVector Buf;
+Buf.append(Prefix->begin(), Prefix->end());
+Buf.append(Suffix->begin(), Suffix->end());
+ReverseMap[StringRef(Buf.begin(), Buf.size())] = *Key;
+  }
+}
+  }
+  return ReverseMap.lookup(DestPath);
+}
Index: clang/include/clang/Lex/HeaderMap.h
===

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a subscriber: bkramer.
DmitryPolukhin added a comment.

In D103142#2783665 , @dexonsmith 
wrote:

> Naively, this sounds like it could be a non-trivial tax on build times. But 
> it looks like it's only called in Clang from `Sema::diagnoseMissingImport`, 
> which only happens on error anyway.

Yes, in Clang `suggestPathToFileForDiagnostics` is used only in 
`Sema::diagnoseMissingImport`. In addition to clangd this function is also used 
in clang-include-fixer but new logic should also work there too (add @bkramer 
author of clang-include-fixer). This diff builds reverse index lazy only when 
it is required so it shouldn't normal build speed.




Comment at: clang/lib/Lex/HeaderSearch.cpp:1855
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (SearchDirs[I].isHeaderMap()) {
+  StringRef SpelledFilename =

bruno wrote:
> Can we save some dir scanning time by adding this logic to the previous loop? 
> Shouldn't get hard to read if you early `continue` for each failed condition.
It seems that unfortunately no, because we need the previous loop to convert 
absolute path into relative and this loop to convert relative path to key in 
header map. Normal header lookup also happens as two lookups: first lookup uses 
header map to convert key to relative path and then another lookup of the 
relative path.



Comment at: clang/lib/Lex/HeaderSearch.cpp:1855
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (SearchDirs[I].isHeaderMap()) {
+  StringRef SpelledFilename =

DmitryPolukhin wrote:
> bruno wrote:
> > Can we save some dir scanning time by adding this logic to the previous 
> > loop? Shouldn't get hard to read if you early `continue` for each failed 
> > condition.
> It seems that unfortunately no, because we need the previous loop to convert 
> absolute path into relative and this loop to convert relative path to key in 
> header map. Normal header lookup also happens as two lookups: first lookup 
> uses header map to convert key to relative path and then another lookup of 
> the relative path.
It seems that unfortunately no, because we need the previous loop to convert 
absolute path into relative and this loop to convert relative path to key in 
header map. Normal header lookup also happens as two lookups: first lookup uses 
header map to convert key to relative path and then another lookup of the 
relative path.



Comment at: clang/lib/Lex/HeaderSearch.cpp:1859
+  if (!SpelledFilename.empty()) {
+Filename = SpelledFilename;
+break;

bruno wrote:
> I guess one of the rationale behind this change is that whatever is consuming 
> your diagnostics is expecting the hmap key entry as an actionable path they 
> could use.
> 
> I wonder whether other consumers of `suggestPathToFileForDiagnostics` expect 
> an actual mapped value or just don't care. If the former, this might be 
> better under some flag.
> 
> @dexonsmith @vsapsai @JDevlieghere @arphaman how does this relate with what 
> you expect out of `suggestPathToFileForDiagnostics`? (If at all).
I think it is safe to change default behaviour without introducing new 
flag/option because I don't expect that anyone really needs value from header 
map search in the source.



Comment at: clang/unittests/Lex/HeaderMapTest.cpp:9
 
-#include "clang/Basic/CharInfo.h"
-#include "clang/Lex/HeaderMap.h"
-#include "clang/Lex/HeaderMapTypes.h"
+#include "HeaderMapTestUtils.h"
 #include "llvm/ADT/SmallString.h"

dexonsmith wrote:
> Splitting this out seems like a great idea, but please split it out to a 
> separate prep commit that's NFC.
Done, https://reviews.llvm.org/D103229


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 348183.
DmitryPolukhin marked 2 inline comments as done.
DmitryPolukhin added a comment.

Split changes into NFC https://reviews.llvm.org/D103229 and the rest


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1834,7 +1834,7 @@
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-// FIXME: Support this search within frameworks and header maps.
+// FIXME: Support this search within frameworks.
 if (!SearchDirs[I].isNormalDir())
   continue;
 
@@ -1848,6 +1848,18 @@
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
 *IsSystem = false;
 
-
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+  // Try resolving resulting filaname via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (SearchDirs[I].isHeaderMap()) {
+  StringRef SpelledFilename =
+  SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+  if (!SpelledFilename.empty()) {
+Filename = SpelledFilename;
+break;
+  }
+}
+  }
+  return path::convert_to_slash(Filename);
 }
Index: clang/lib/Lex/HeaderMap.cpp
===
--- clang/lib/Lex/HeaderMap.cpp
+++ clang/lib/Lex/HeaderMap.cpp
@@ -240,3 +240,26 @@
 return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (ReverseMap.empty()) {
+const HMapHeader &Hdr = getHeader();
+unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+for (unsigned i = 0; i != NumBuckets; ++i) {
+  HMapBucket B = getBucket(i);
+  if (B.Key == HMAP_EmptyBucketKey)
+continue;
+
+  Optional Key = getString(B.Key);
+  Optional Prefix = getString(B.Prefix);
+  Optional Suffix = getString(B.Suffix);
+  if (Key && Prefix && Suffix) {
+SmallVector Buf;
+Buf.append(Prefix->begin(), Prefix->end());
+Buf.append(Suffix->begin(), Suffix->end());
+ReverseMap[StringRef(Buf.begin(), Buf.size())] = *Key;
+  }
+}
+  }
+  return Revers

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

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

dexonsmith added a comment.

Naively, this sounds like it could be a non-trivial tax on build times. But it 
looks like it's only called in Clang from `Sema::diagnoseMissingImport`, which 
only happens on error anyway.




Comment at: clang/unittests/Lex/HeaderMapTest.cpp:9
 
-#include "clang/Basic/CharInfo.h"
-#include "clang/Lex/HeaderMap.h"
-#include "clang/Lex/HeaderMapTypes.h"
+#include "HeaderMapTestUtils.h"
 #include "llvm/ADT/SmallString.h"

Splitting this out seems like a great idea, but please split it out to a 
separate prep commit that's NFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added subscribers: JDevlieghere, vsapsai.
bruno added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:1851
 
-
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+  // Try resolving resulting filaname via reverse search in header maps,
+  // key from header name is user prefered name for the include file.

-> `filename`



Comment at: clang/lib/Lex/HeaderSearch.cpp:1855
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+if (SearchDirs[I].isHeaderMap()) {
+  StringRef SpelledFilename =

Can we save some dir scanning time by adding this logic to the previous loop? 
Shouldn't get hard to read if you early `continue` for each failed condition.



Comment at: clang/lib/Lex/HeaderSearch.cpp:1859
+  if (!SpelledFilename.empty()) {
+Filename = SpelledFilename;
+break;

I guess one of the rationale behind this change is that whatever is consuming 
your diagnostics is expecting the hmap key entry as an actionable path they 
could use.

I wonder whether other consumers of `suggestPathToFileForDiagnostics` expect an 
actual mapped value or just don't care. If the former, this might be better 
under some flag.

@dexonsmith @vsapsai @JDevlieghere @arphaman how does this relate with what you 
expect out of `suggestPathToFileForDiagnostics`? (If at all).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

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

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 347864.
DmitryPolukhin added a comment.

Fix linter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderMapTest.cpp
  clang/unittests/Lex/HeaderMapTestUtils.h
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/unittests/Lex/HeaderMapTestUtils.h
===
--- /dev/null
+++ clang/unittests/Lex/HeaderMapTestUtils.h
@@ -0,0 +1,100 @@
+//===- unittests/Lex/HeaderMapTestUtils.h - HeaderMap utils ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_UNITTESTS_LEX_HEADERMAPTESTUTILS_H
+#define LLVM_CLANG_UNITTESTS_LEX_HEADERMAPTESTUTILS_H
+
+#include "clang/Basic/CharInfo.h"
+#include "clang/Lex/HeaderMap.h"
+#include "clang/Lex/HeaderMapTypes.h"
+#include "llvm/Support/SwapByteOrder.h"
+#include 
+
+namespace clang {
+namespace test {
+
+// Lay out a header file for testing.
+template  struct HMapFileMock {
+  HMapHeader Header;
+  HMapBucket Buckets[NumBuckets];
+  unsigned char Bytes[NumBytes];
+
+  void init() {
+memset(this, 0, sizeof(HMapFileMock));
+Header.Magic = HMAP_HeaderMagicNumber;
+Header.Version = HMAP_HeaderVersion;
+Header.NumBuckets = NumBuckets;
+Header.StringsOffset = sizeof(Header) + sizeof(Buckets);
+  }
+
+  void swapBytes() {
+using llvm::sys::getSwappedBytes;
+Header.Magic = getSwappedBytes(Header.Magic);
+Header.Version = getSwappedBytes(Header.Version);
+Header.NumBuckets = getSwappedBytes(Header.NumBuckets);
+Header.StringsOffset = getSwappedBytes(Header.StringsOffset);
+  }
+
+  std::unique_ptr getBuffer() {
+return llvm::MemoryBuffer::getMemBuffer(
+StringRef(reinterpret_cast(this), sizeof(HMapFileMock)),
+"header",
+/* RequresNullTerminator */ false);
+  }
+};
+
+template  struct HMapFileMockMaker {
+  FileTy &File;
+  unsigned SI = 1;
+  unsigned BI = 0;
+  HMapFileMockMaker(FileTy &File) : File(File) {}
+
+  unsigned addString(StringRef S) {
+assert(SI + S.size() + 1 <= sizeof(File.Bytes));
+std::copy(S.begin(), S.end(), File.Bytes + SI);
+auto OldSI = SI;
+SI += S.size() + 1;
+return OldSI;
+  }
+
+  void addBucket(StringRef Str, unsigned Key, unsigned Prefix,
+

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 347862.
DmitryPolukhin added a comment.

Fix clang-format issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderMapTest.cpp
  clang/unittests/Lex/HeaderMapTestUtils.h
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/unittests/Lex/HeaderMapTestUtils.h
===
--- /dev/null
+++ clang/unittests/Lex/HeaderMapTestUtils.h
@@ -0,0 +1,100 @@
+//===- unittests/Lex/HeaderMapTestUtils.h - HeaderMap utils ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_UNITTESTS_LEX_HEADERMAPTESTUTILS_H
+#define LLVM_CLANG_UNITTESTS_LEX_HEADERMAPTESTUTILS_H
+
+#include "clang/Basic/CharInfo.h"
+#include "clang/Lex/HeaderMap.h"
+#include "clang/Lex/HeaderMapTypes.h"
+#include "llvm/Support/SwapByteOrder.h"
+#include 
+
+namespace clang {
+namespace test {
+
+// Lay out a header file for testing.
+template  struct HMapFileMock {
+  HMapHeader Header;
+  HMapBucket Buckets[NumBuckets];
+  unsigned char Bytes[NumBytes];
+
+  void init() {
+memset(this, 0, sizeof(HMapFileMock));
+Header.Magic = HMAP_HeaderMagicNumber;
+Header.Version = HMAP_HeaderVersion;
+Header.NumBuckets = NumBuckets;
+Header.StringsOffset = sizeof(Header) + sizeof(Buckets);
+  }
+
+  void swapBytes() {
+using llvm::sys::getSwappedBytes;
+Header.Magic = getSwappedBytes(Header.Magic);
+Header.Version = getSwappedBytes(Header.Version);
+Header.NumBuckets = getSwappedBytes(Header.NumBuckets);
+Header.StringsOffset = getSwappedBytes(Header.StringsOffset);
+  }
+
+  std::unique_ptr getBuffer() {
+return llvm::MemoryBuffer::getMemBuffer(
+StringRef(reinterpret_cast(this), sizeof(HMapFileMock)),
+"header",
+/* RequresNullTerminator */ false);
+  }
+};
+
+template  struct HMapFileMockMaker {
+  FileTy &File;
+  unsigned SI = 1;
+  unsigned BI = 0;
+  HMapFileMockMaker(FileTy &File) : File(File) {}
+
+  unsigned addString(StringRef S) {
+assert(SI + S.size() + 1 <= sizeof(File.Bytes));
+std::copy(S.begin(), S.end(), File.Bytes + SI);
+auto OldSI = SI;
+SI += S.size() + 1;
+return OldSI;
+  }
+
+  void addBucket(StringRef Str, unsigned Key, unsigned Prefix,
+

[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin created this revision.
DmitryPolukhin added reviewers: dexonsmith, bruno, rsmith.
DmitryPolukhin added a project: clang.
Herald added subscribers: usaxena95, ormris, kadircet, arphaman.
DmitryPolukhin requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

suggestPathToFileForDiagnostics is actively used in clangd for converting
an absolute path to a header file to a header name as it should be spelled
in the sources. Current approach converts absolute path to relative path.
This diff implements missing logic that makes a reverse lookup from the
relative path to the key in the header map that should be used in the sources.

Test Plan: check-clang


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103142

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderMapTest.cpp
  clang/unittests/Lex/HeaderMapTestUtils.h
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
@@ -45,6 +46,21 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+std::unique_ptr Buf) {
+VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+ llvm::sys::fs::file_type::regular_file);
+auto FE = FileMgr.getFile(Filename, true);
+assert(FE);
+
+// Test class supports only one HMap at a time.
+assert(!HMap);
+HMap = HeaderMap::Create(*FE, FileMgr);
+auto DL =
+DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -55,6 +71,7 @@
   std::shared_ptr TargetOpts;
   IntrusiveRefCntPtr Target;
   HeaderSearch Search;
+  std::unique_ptr HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
@@ -136,5 +153,31 @@
 "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template 
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"d.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/unittests/Lex/HeaderMapTestUtils.h
===
--- /dev/null
+++ clang/unittests/Lex/HeaderMapTestUtils.h
@@ -0,0 +1,100 @@
+//===- unittests/Lex/HeaderMapTestUtils.h - HeaderMap utils ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_UNITTESTS_LEX_HEADERMAPTESTUTILS_H
+#define LLVM_CLANG_UNITTESTS_LEX_HEADERMAPTESTUTILS_H
+
+#include "clang/Basic/CharInfo.h"
+#include "clang/Lex/HeaderMap.h"
+#include "clang/Lex/HeaderMapTypes.h"
+#include "llvm/Support/SwapByteOrder.h"
+#include 
+
+namespace clang {
+namespace test {
+
+// Lay out a header file for testing.
+template  struct HMapFileMock {
+  HMapHeader Header;
+  HMapBucket Buckets[NumBuckets];
+  unsigned char Bytes[NumBytes];
+
+  void init() {
+memset(this, 0, sizeof(HMapFileMock));
+Header.Magic = HMAP_HeaderMagicNumber;
+Header.Version = HMAP_HeaderVersion;
+Header.NumBuckets = NumBuckets;
+Header.StringsOffset = sizeof(Header) + sizeof(Buckets);
+  }
+
+  void swapBytes() {
+using llvm::sys::getSwappedBytes;
+Header.Magic = getSwappedBytes(Header.Magic);
+Header.Version = getSwappedBytes(Header.Version);
+Header.NumBuckets = getSwappedBytes(Header.NumBuckets);
+Header.StringsOffset = getSwappedBytes(Header.StringsOffset);
+  }
+
+  std::unique_ptr getBuffer() {
+return llvm::MemoryBuffer::getMemBuffer(
+StringRef(reinterpret_cast(this), sizeof(H