[PATCH] D39050: Add index-while-building support to Clang

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Index/IndexDataStoreSymbolUtils.h:13
+
+#include "indexstore/indexstore.h"
+#include "clang/Index/IndexSymbol.h"
----------------
It looks to me like this header, `"indexstore/indexstore.h"`, and `IndexDataStoreUtils.cpp` are utilities just for the C API, so could we take it out of here as well?


================
Comment at: lib/Driver/ToolChains/Clang.cpp:3597
+
+  if (const char *IdxStorePath = ::getenv("CLANG_PROJECT_INDEX_PATH")) {
+    CmdArgs.push_back("-index-store-path");
----------------
What is this environment variable used for? And why does it imply the other two flags? 


================
Comment at: lib/Frontend/CompilerInstance.cpp:1153
+  // Pass along the GenModuleActionWrapper callback
+  auto wrapGenModuleAction = ImportingInstance.getGenModuleActionWrapper();
+  Instance.setGenModuleActionWrapper(wrapGenModuleAction);
----------------
Please start your variable names with uppercase (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).


================
Comment at: lib/Index/IndexRecordHasher.cpp:103
+      COMBINE_HASH(Hasher.hash(param->getType()));
+    }
+    return Hash;
----------------
Should you hash the return type as well?


================
Comment at: lib/Index/IndexRecordHasher.cpp:137
+    Loc = SM.getFileLoc(Loc);
+    const std::pair<FileID, unsigned> &Decomposed = SM.getDecomposedLoc(Loc);
+    const FileEntry *FE = SM.getFileEntryForID(Decomposed.first);
----------------
No need to use a reference type here.


================
Comment at: lib/Index/IndexRecordHasher.cpp:146
+    if (IncludeOffset) {
+      // Use the offest into the FileID to represent the location.  Using
+      // a line/column can cause us to look back at the original source file,
----------------
offset


================
Comment at: lib/Index/IndexRecordHasher.cpp:204
+    if (Q.hasConst())
+      qVal |= 0x1;
+    if (Q.hasVolatile())
----------------
You can use `Qualifiers::Const` here or make your own enum instead of raw constants.


================
Comment at: lib/Index/IndexRecordHasher.cpp:255
+      continue;
+    }
+
----------------
There are other types in Clang which aren't hashed it seems. Can you add a general FIXME for them here?


================
Comment at: lib/Index/IndexRecordWriter.cpp:278
+  // Write the record header.
+  auto *State = new RecordState(RecordPath.str());
+  Record = State;
----------------
It might be better to forward declare `RecordState` in the header, use `unique_ptr<RecordState>` field instead of `void *` in the class and use `llvm::make_unique` here. Then you can remove the `ScopedDelete` RAII struct in the next function.


================
Comment at: lib/Index/IndexUnitWriter.cpp:30
+
+class IndexUnitWriter::PathStorage {
+  std::string WorkDir;
----------------
What is this class responsible for?


================
Comment at: lib/Index/IndexingAction.cpp:686
+    if (RecordWriter.writeRecord(FE->getName(), Rec, Error, &RecordFile)) {
+      unsigned DiagID = Diag.getCustomDiagID(DiagnosticsEngine::Error,
+                                             "failed writing record '%0': %1");
----------------
We might want to start using a new diagnostic group for index-while-building errors instead of the custom ones.


================
Comment at: lib/Index/IndexingContext.cpp:162
+  // treated as system one.
+  if (path == "/")
+    path = StringRef();
----------------
It might be worth investigating if you can use any of the LLVM's path APIs here instead of doing a UNIX-specific check.


https://reviews.llvm.org/D39050