sammccall 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()));
----------------
kbobyrev wrote:
> 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.
If we remove the else, then YAMLContents is no longer in scope.
================
Comment at: clangd/index/Serialization.cpp:412
+
+std::unique_ptr<SymbolIndex> loadIndex(llvm::StringRef SymbolFilename,
+ llvm::ArrayRef<std::string> URISchemes,
----------------
kbobyrev wrote:
> Would `llvm::Option<SymbolIndex>` be better here? I used this in the
> benchmark patch, that might be semantically better than returning `nullptr`.
SymbolIndex is abstract.
================
Comment at: clangd/indexer/IndexerMain.cpp:68
+ "binary RIFF format")),
+ llvm::cl::init(IndexFileFormat::YAML));
----------------
kbobyrev wrote:
> Maybe change default value to `IndexFileFormat::RIFF`? `IndexFileOut::Format`
> is set to `IndexFileFormat::RIFF;` in `Serialization.h` by default.
I agree we should switch it, but this refactoring patch isn't the place to
change the behavior of binaries.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52453
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
