This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Serialization support for RelationSlab
ClosedPublic

Authored by nridge on May 25 2019, 7:21 PM.

Event Timeline

nridge created this revision.May 25 2019, 7:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2019, 7:22 PM
kadircet added inline comments.May 27 2019, 2:48 AM
clang-tools-extra/clangd/index/Serialization.cpp
29

no need for errors, use llvm_unreachable instead.

45

same for error

73

I believe reader should not care about the high level representation of data. therefore error state should be internal and only set if the low level read has failed.

high level reads are out of Readers scope and should be propagated by the caller.

386

could you also add a comment saying, we might prefer a packed representation if need arises in future?

clang-tools-extra/clangd/index/Serialization.h
81

why not start from zero?

Also let's change the index::SymbolRole in Relation with this one.

81

why not just store baseof ? do we need both directions for typehierarchy?

clang-tools-extra/clangd/unittests/SerializationTests.cpp
243

let's incorporate these data into existing yaml string at top of the file, I believe you just need to add `--- !Relations``` section

nridge marked an inline comment as done.May 29 2019, 8:51 AM
nridge added inline comments.
clang-tools-extra/clangd/index/Serialization.h
81

Also let's change the index::SymbolRole in Relation with this one.

You previously asked for the opposite change in this comment :)

Did your opinion change?

nridge updated this revision to Diff 202370.May 30 2019, 10:33 PM
nridge marked 8 inline comments as done.

Addressed most review comments

clang-tools-extra/clangd/index/Serialization.h
81

why not start from zero?

The original motivation was so that we had a distinct value to return in case of an error. However, now that we use llvm_unreachable that doesn't seem to be necessary any more.

81

why not just store baseof ? do we need both directions for typehierarchy?

In the future, we may want to be able to look up supertypes in the index as well (for example, to handle scenarios where a type that is only forward-declared in the current TU is queried).

However, we can add that later, and just focus on subtypes for now, which only require BaseOf.

kadircet accepted this revision.May 31 2019, 1:46 AM

LG, thanks for the patch!

clang-tools-extra/clangd/index/Serialization.cpp
34

nit: no need for braces

44

nit: braces

clang-tools-extra/clangd/index/Serialization.h
81

ahaha good catch :D for some reason I thought we were also storing our own struct in Symbol for SmybolInfo but apparently we are not. Just ignore that one.

This revision is now accepted and ready to land.May 31 2019, 1:46 AM
This revision was automatically updated to reflect the committed changes.
nridge marked 4 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2019, 10:04 PM