[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D72746#1827608 <https://reviews.llvm.org/D72746#1827608>, @kbobyrev wrote:

> I started using TokenBuffer, but I ran into the following issue: when I'm
>  creating `TokenBuffer` and `TokenCollector`, they do not contain any tokens.
>  `Preprocessor` does not seem to have a non-null Lexer instance, `TokenWatcher`
>  (set in `TokenCollector`) is not triggered anywhere and neither is
>  `Lexer::Lex`. I don't have much familiarity with the code and I looked at the
>  other usages of `TokenBuffer` but didn't figure out what's wrong with the code
>  in this patch. I suspect the lexer in Preprocessor should be re-initialized
>  somehow? I'm certainly missing something here.


right, I forgot that `TokenCollector` must be constructed before starting to process the file, and unfortunately in some
code paths we create a `SymbolCollector` long after parsing the file. You can make use of `syntax::Tokenize` for now,
as we only care about SpelledTokens in the file. Please leave a TODO though saying that, we should rather pass one from
the caller of SymbolCollector.

Sorry for the inconvenience.



================
Comment at: clang-tools-extra/clangd/index/Ref.h:36
+  // one below.
+  Implicit = 1 << 3,
+  All = Declaration | Definition | Reference | Implicit,
----------------
kbobyrev wrote:
> kadircet wrote:
> > kadircet wrote:
> > > instead of doing that, could we rather de-couple two enums completely and have a `symbolRoleToRefKind`(and vice-versa) method instead(we already have some customization in `toRefKind` now) ?
> > > 
> > > as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd)
> > note that this would also require a bump to version of `on-disk` index in clangd/index/Serialization.cpp, as old information regarding `RefKind` won't be usable anymore.
> > instead of doing that, could we rather de-couple two enums completely and have a symbolRoleToRefKind(and vice-versa) method instead(we already have some customization in toRefKind now) ?
> > as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd)
> 
> Makes sense, will do!
> 
> > note that this would also require a bump to version of on-disk index in clangd/index/Serialization.cpp, as old information regarding RefKind won't be usable anymore.
> 
> I checked the Serialization code and the serialization code should be OK as long as `RefKind` stays one byte. Can you please elaborate on this?
> 
> Do you mean that the index should be generated again after this change because it would no longer be valid? (this one I understand)
> Or do you mean there should be some other change in the code for me to do to land this patch?
> Do you mean that the index should be generated again after this change because it would no longer be valid? (this one I understand)

yes i meant this


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72746/new/

https://reviews.llvm.org/D72746