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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
