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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.
Herald added a subscriber: jdoerfert.


================
Comment at: include/clang/Frontend/FrontendOptions.h:377
 
+  /// The path to write index data to
+  std::string IndexStorePath;
----------------
Please end comments with a period.


================
Comment at: include/clang/Frontend/FrontendOptions.h:380
+  /// Whether to ignore system files when writing out index data
+  unsigned IndexIgnoreSystemSymbols : 1;
+  /// Whether to include the codegen name of symbols in the index data
----------------
Would it make more sense to flip this boolean to positive?  "IndexIncludeSystemSymbols"?


================
Comment at: lib/Index/IndexingAction.cpp:102
+  /// \param CI The compiler instance used to process the input
+  /// \returns the created IndexASTConsumer.
+  virtual std::unique_ptr<IndexASTConsumer>
----------------
Please don't duplicate the information from the signature in comments.  No need to say that this function returns an IndexASTConsumer (twice, in the first sentence and in the \returns clause), the code already says that.

Also, "The compiler instance used to process the input" does not mean much to me either.


================
Comment at: lib/Index/IndexingAction.cpp:154
+    return llvm::make_unique<MultiplexConsumer>(std::move(Consumers));
+  };
+
----------------
No semicolon.


================
Comment at: lib/Index/IndexingAction.cpp:163
+    }
+  };
 };
----------------
No semicolon.


================
Comment at: lib/Index/IndexingAction.cpp:186
+  void finish(CompilerInstance &CI) override { DataConsumer->finish(); }
+};
+
----------------
No semicolon.


================
Comment at: lib/Index/IndexingAction.cpp:275
+/// Construct a \c UnitDetails from the invocation associated with the provided
+/// \c CompilerInstance and the provided sysroot path.
+static index::UnitDetails getUnitDetails(const CompilerInstance &CI,
----------------
Please don't duplicate type information from the signature in the comment.



================
Comment at: lib/Index/IndexingAction.cpp:283
+    OutputFile = CI.getFrontendOpts().Inputs[0].getFile();
+    OutputFile += ".o";
+  }
----------------
I don't understand... this is not really the user-specified output file.


================
Comment at: lib/Index/IndexingAction.cpp:303
+
+/// Construct a \c UnitDetails for the given module file.
+static index::UnitDetails getUnitDetails(serialization::ModuleFile &Mod,
----------------
Please don't duplicate type information from the signature in the comment.


================
Comment at: lib/Index/IndexingContext.h:40
+/// Tracks the current system root path and computes and caches whether a
+/// file is considered a system file or not
+class SystemFileCache {
----------------
Please add a period at the end of the comment.


================
Comment at: lib/Index/IndexingContext.h:44
+  // Records whether a directory entry is system or not.
+  llvm::DenseMap<const DirectoryEntry *, bool> DirEntries;
+  // Keeps track of the last check for whether a FileID is system or
----------------
DirEntries => IsSystemDirEntry?


================
Comment at: lib/Index/IndexingContext.h:46
+  // Keeps track of the last check for whether a FileID is system or
+  // not. This is used to speed up isSystemFile() call.
+  std::pair<FileID, bool> LastFileCheck;
----------------
Triple slashes for doc comments.


================
Comment at: lib/Index/IndexingContext.h:46
+  // Keeps track of the last check for whether a FileID is system or
+  // not. This is used to speed up isSystemFile() call.
+  std::pair<FileID, bool> LastFileCheck;
----------------
gribozavr wrote:
> Triple slashes for doc comments.
Unclear how a boolean can keep track of the last check.

Did you mean "Whether the file is a system file or not.  This value is a cache."  If so, please rename the variable to something like IsSystemFileCache.


================
Comment at: test/Index/Core/index-source.mm:2
 // RUN: c-index-test core -print-source-symbols -- %s -target x86_64-apple-macosx10.7 | FileCheck %s
+// RUN: c-index-test core -print-source-unit -- %s -target x86_64-apple-macosx10.7 | FileCheck -check-prefixes=CHECK %s
 
----------------
No need to specify check-prefixes=CHECK.


================
Comment at: test/Index/Core/index-unit.mm:1
+// RUN: rm -rf %t.mcp
+// RUN: c-index-test core -print-source-unit -- -arch x86_64 -mmacosx-version-min=10.7 -c %s -o %t.o -isystem %S/Inputs/sys -fmodules -fmodules-cache-path=%t.mcp -Xclang -fdisable-module-hash -I %S/Inputs/module -I %S/Inputs | FileCheck %s
----------------
This test is very difficult to read... it is just a dump of random internal data structures... what do you think about converting it to a unit test?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D39050/new/

https://reviews.llvm.org/D39050