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