This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sammccall on Sep 25 2018, 1:04 AM.

Details

Summary

Interface is in one file, implementation in two as they have little in common.
A couple of ad-hoc YAML functions left exposed:

  • symbol -> YAML I expect to keep for tools like dexp
  • YAML -> symbol is used for the MR-style indexer, I think we can eliminate this (merge-on-the-fly, else use a different serialization)

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Sep 25 2018, 1:04 AM
sammccall updated this revision to Diff 166828.Sep 25 2018, 1:39 AM

Update tests.

kbobyrev added inline comments.Sep 25 2018, 6:29 AM
clangd/index/Serialization.cpp
407 ↗(On Diff #166828)

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.

412 ↗(On Diff #166828)

Would llvm::Option<SymbolIndex> be better here? I used this in the benchmark patch, that might be semantically better than returning nullptr.

clangd/indexer/IndexerMain.cpp
68 ↗(On Diff #166828)

Maybe change default value to IndexFileFormat::RIFF? IndexFileOut::Format is set to IndexFileFormat::RIFF; in Serialization.h by default.

sammccall added inline comments.Sep 25 2018, 6:45 AM
clangd/index/Serialization.cpp
407 ↗(On Diff #166828)

If we remove the else, then YAMLContents is no longer in scope.

412 ↗(On Diff #166828)

SymbolIndex is abstract.

clangd/indexer/IndexerMain.cpp
68 ↗(On Diff #166828)

I agree we should switch it, but this refactoring patch isn't the place to change the behavior of binaries.

kbobyrev accepted this revision.Sep 25 2018, 7:07 AM

Ah, I see. Thanks for clarifying! I was wondering if we should also rename -yaml-symbol-file flag to something like -static-symbol-file (but, as you said, it's something to do in another patch).

LGTM

This revision is now accepted and ready to land.Sep 25 2018, 7:07 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
clang-tools-extra/trunk/clangd/index/SymbolYAML.h