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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to