[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:96
+    // Sort input for determinism (index is used as a tiebreaker).
+    llvm::sort(OriginalPaths.begin(), OriginalPaths.end());
+    for (size_t I = 0; I < Filenames.size(); ++I) {
----------------
`OriginalPaths` is empty at this point, did we intend to sort `Filenames` instead?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:99
+      OriginalPaths.emplace_back(Strings.save(Filenames[I]));
+      StringRef Path = Strings.save(OriginalPaths.back().lower());
+      Paths.push_back({Path, I});
----------------
Given that we do an extra allocation when using `lower()` anyway, an extra copy into `Strings` is redundant.
Do we really need the arena at this point? It adds extra copies that we might not want.
It might give better memory locality, though, so it may be faster to use it overall, so up to you.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:224
+    if (It == Paths.end())
+      return *--It;
+    // Have to choose between It and It-1
----------------
Maybe add an assertion that `Idx` is non-empty?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:308
+               llvm::sys::path::extension(Filename).substr(1));
+      if (OldLang && NewLang != OldLang) {
+        Base.CommandLine.push_back("-x");
----------------
It feels like this heuristics only works for headers and files without extension (i.e. probably also headers).
E.g., if we have a .cpp file and .c file, then trying to infer args for .c file from .cpp file is probably the wrong thing to do. And using Fortran flags for C or C++ is certainly the wrong thing to do.

It seems transferring flags between different languages is never fine, except for C/C++ headers. WDYT?


Repository:
  rC Clang

https://reviews.llvm.org/D45006