[PATCH] D47905: [ThinLTO] Parse module summary index from assembly

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

I've just had time to skim a few files; feel free to ping me privately if I don't continue the review later today.



================
Comment at: include/llvm/AsmParser/Parser.h:87
+/// \param DataLayoutString Override datalayout in the llvm assembly.
+std::pair<std::unique_ptr<Module>, std::unique_ptr<ModuleSummaryIndex>>
+parseAssemblyFileWithIndex(StringRef Filename, SMDiagnostic &Error,
----------------
I wonder if it's worth adding a simple named struct for this.


================
Comment at: include/llvm/AsmParser/Parser.h:136
+
+/// parseSummaryIndexAssemblyFile is a wrapper around this function.
+/// Parse LLVM Assembly for summary index from a MemoryBuffer.
----------------
I feel like this comment should come after you describe what the function does.  I think the next line should be first (and have an empty line after, so that it's the only thing showing up in doxygen "brief").


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:775-776
+  // don't have the string owned elsewhere (e.g. the Strtab on a module).
+  StringSaver Saver;
+  BumpPtrAllocator Alloc;
+
----------------
The comment suggests these aren't always used.  Should they be in an optional?


================
Comment at: lib/AsmParser/LLLexer.cpp:722
 
+  // Summary index keywords
+  KEYWORD(path);
----------------
Missing period.


Repository:
  rL LLVM

https://reviews.llvm.org/D47905