[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Awesome  :-)



================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:220
 
-// CommandIndex does the real work: given a filename, it produces the best
-// matching TransferableCommand by matching filenames. Basic strategy:
+// FileProximityIndex chooses the proxy file that has a compile command in a
+// compilation database when given a file that is not in the database. Basic
----------------
This summary is a bit unclear to me. (too many clauses, maybe too abstract).
And the high level heuristics are hidden a bit below the implementation ideas.

Maybe

```
Given a filename, FileProximityIndex picks the best matching file from the underlying DB. This is the proxy file whose CompileCommand will be reused.

The heuristics incorporate file name, extension, and directory structure.

Strategy:
...```


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:220
 
-// CommandIndex does the real work: given a filename, it produces the best
-// matching TransferableCommand by matching filenames. Basic strategy:
+// FileProximityIndex chooses the proxy file that has a compile command in a
+// compilation database when given a file that is not in the database. Basic
----------------
sammccall wrote:
> This summary is a bit unclear to me. (too many clauses, maybe too abstract).
> And the high level heuristics are hidden a bit below the implementation ideas.
> 
> Maybe
> 
> ```
> Given a filename, FileProximityIndex picks the best matching file from the underlying DB. This is the proxy file whose CompileCommand will be reused.
> 
> The heuristics incorporate file name, extension, and directory structure.
> 
> Strategy:
> ...```
nit: I'd prefer `FileIndex` or `FilenameIndex` here - "proximity" emphasizes directory structure over stem/extension, which are pretty important!


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:232
-      : Commands(std::move(AllCommands)), Strings(Arena) {
-    // Sort commands by filename for determinism (index is a tiebreaker later).
-    llvm::sort(
----------------
restore this comment?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:338
       S.Preferred = PreferredLanguage == types::TY_INVALID ||
-                    PreferredLanguage == Commands[S.Index].Type;
+                    PreferredLanguage == guessType(Paths[S.Index].first);
       S.Points = Candidate.second;
----------------
hmm, I would have thought we'd store the values of guessType() when building the index. I guess it doesn't matter, it just seems surprising to see this call here.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:338
       S.Preferred = PreferredLanguage == types::TY_INVALID ||
-                    PreferredLanguage == Commands[S.Index].Type;
+                    PreferredLanguage == guessType(Paths[S.Index].first);
       S.Points = Candidate.second;
----------------
sammccall wrote:
> hmm, I would have thought we'd store the values of guessType() when building the index. I guess it doesn't matter, it just seems surprising to see this call here.
you're calling foldType(guessType(...)) on the query, do you need to fold here too?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:417
     bool TypeCertain;
     auto Lang = guessType(Filename, &TypeCertain);
     if (!TypeCertain)
----------------
this guessType/foldType call should be folded into Index.chooseProxy now I think - Index explicitly knows that the language it deals with must be derived from the filename.


Repository:
  rC Clang

https://reviews.llvm.org/D51314