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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
