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