kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land.
LG, thanks for the patch! ================ Comment at: clang-tools-extra/clangd/index/Serialization.cpp:34 + switch (Role) { + case index::SymbolRole::RelationBaseOf: { + return RelationKind::BaseOf; ---------------- nit: no need for braces ================ Comment at: clang-tools-extra/clangd/index/Serialization.cpp:44 + switch (Kind) { + case RelationKind::BaseOf: { + return index::SymbolRole::RelationBaseOf; ---------------- nit: braces ================ Comment at: clang-tools-extra/clangd/index/Serialization.h:81 +// Used for serializing SymbolRole as used in Relation. +enum class RelationKind : uint8_t { ChildOf = 1, BaseOf }; +llvm::Expected<RelationKind> symbolRoleToRelationKind(index::SymbolRole); ---------------- nridge wrote: > nridge wrote: > > nridge wrote: > > > kadircet wrote: > > > > kadircet wrote: > > > > > why not start from zero? > > > > > > > > > > Also let's change the `index::SymbolRole` in `Relation` with this one. > > > > why not just store baseof ? do we need both directions for > > > > typehierarchy? > > > > Also let's change the index::SymbolRole in Relation with this one. > > > > > > You previously asked for the opposite change in [this > > > comment](https://reviews.llvm.org/D59407?id=190785#inline-525734) :) > > > > > > Did your opinion change? > > > 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. > > 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. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62459/new/ https://reviews.llvm.org/D62459 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits