[PATCH] D52453: [clangd] Merge binary + YAML serialization behind a (mostly) common interface.

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clangd/index/Serialization.cpp:407
+  } else {
+    return makeError("Not a RIFF file and failed to parse as YAML: " +
+                     llvm::toString(YAMLContents.takeError()));
----------------
nit: probably omit the last `else` so that it's easier to read? This is not exactly [[ https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return | Early Exits & `else` after `return` ]] case, but looks rather similar.


================
Comment at: clangd/index/Serialization.cpp:412
+
+std::unique_ptr<SymbolIndex> loadIndex(llvm::StringRef SymbolFilename,
+                                       llvm::ArrayRef<std::string> URISchemes,
----------------
Would `llvm::Option<SymbolIndex>` be better here? I used this in the benchmark patch, that might be semantically better than returning `nullptr`.


================
Comment at: clangd/indexer/IndexerMain.cpp:68
+                                       "binary RIFF format")),
+           llvm::cl::init(IndexFileFormat::YAML));
 
----------------
Maybe change default value to `IndexFileFormat::RIFF`? `IndexFileOut::Format` is set to `IndexFileFormat::RIFF;` in `Serialization.h` by default.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52453