[PATCH] D45006: [Tooling] A CompilationDatabase wrapper that infers header commands.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:26
+// their compile commands. If getAllFilenames() is empty, no inference occurs.
+//
+//===----------------------------------------------------------------------===//
----------------
Maybe add a comment describing the use-cases we had in mind while designing these heuristics? 
  - .cpp and .h files usually have the same (or slightly modified) name, usually the prefix match,
  - LLVM (and other) codebases that put .h and .cpp files into different directories,
  - even random matches are better than arbitrary defaults,
  - ...


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:83
+template <bool Reverse> int prefixCompare(StringRef S, StringRef Prefix) {
+  if (S.size() >= Prefix.size())
+    return Reverse ? rMemCompare(S.end(), Prefix.end(), Prefix.size())
----------------
Summarizing the offline discussion, we could exclude suffix matches from the initial version. This would make the code much simpler, and it seems most C++ projects we know of would actually work with prefix-only matches.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:115
+    for (auto F : getAllFiles())
+      Paths.emplace_back(Strings.save(F), 0);
+    finalizeIndex();
----------------
This class seems to do two somewhat orthogonal things: 
  - build and query the index structure for the paths,
  - handle queries to inner CDB and patch the compile commands for other files accordingly.

Maybe we could extract the code that handles the index into a separate class?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:141
+  void finalizeIndex() {
+    llvm::sort(Paths.begin(), Paths.end());
+    for (size_t I = 0; I < Paths.size(); ++I) {
----------------
Maybe store lower-cased paths in the index and compare case-insensitively when querying?
Having slight case mismatches is not uncommon and case-sensitivity shouldn't ever be the defining factor for this kind of heuristics.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:147
+      auto Dir = ++sys::path::rbegin(Path), DirEnd = sys::path::rend(Path);
+      // Index up to 4 path components.
+      for (int J = 0; J < 4 && Dir != DirEnd; ++J, ++Dir)
----------------
Same as prev. comment, maybe comment on why 4 was chosen here? Maybe use a named constant?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:187
+    StringRef Stem = sys::path::stem(Filename);
+    llvm::SmallVector<StringRef, 2> Dirs; // Only look up the last 2.
+    llvm::StringRef Prefix;
----------------
Maybe add a comment why 2 is chosen here? Also, maybe use a named constant?


Repository:
  rC Clang

https://reviews.llvm.org/D45006