[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[Dmitri Gribenko via Phabricator via cfe-commits]

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

I'm not an expert in this code, but it looks reasonable.



================
Comment at: clang-tools-extra/clangd/Headers.h:155
+  /// \param IncludingFile Used to shorten the include for headers in the same
+  /// directory.
+  ///
----------------
Please don't write what something is used for, write what it is. You can explain how it affects the output of the function of course, but don't just say "used for" -- like what you did in clang/include/clang/Lex/HeaderSearch.h.


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:215
 
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");
----------------
This test name should probably mention the fact that the directory is on the include path.


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:225
 
 TEST_F(HeadersTest, NotShortenedInclude) {
   std::string BarHeader =
----------------
The test name probably should be adjusted.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1683
   unsigned BestPrefixLength = 0;
-  unsigned BestSearchDir;
-
-  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-    // FIXME: Support this search within frameworks and header maps.
-    if (!SearchDirs[I].isNormalDir())
-      continue;
-
-    StringRef Dir = SearchDirs[I].getDir()->getName();
+  auto CheckDir = [&](llvm::StringRef Dir) {
     llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
----------------
I'd suggest to spell the return type, and document what it means.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63295