sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:89 + Result.try_emplace(Token(Token::Kind::Trigram, E.first.str()), + std::move(E.second)); + TrigramDocs.clear(); ---------------- kadircet wrote: > sammccall wrote: > > the move here does nothing, we're passing as ArrayRef > > > > did you want to clear the map values (vectors) as you went, too? > > > > (this gets a bit verbose, but you could pull out a function template since > > all 4 cases are so regular) > > did you want to clear the map values (vectors) as you went, too? > > yes that's the idea. would you prefer an explicit destructor call? > > > (this gets a bit verbose, but you could pull out a function template since > > all 4 cases are so regular) > > the problem is 1 of these is a densemap, other three is a stringmap, and > onther is a vector. densemap and the stringmap doesn't really go together > because there's no common method to get the key (densemap has `first` as a > field, or `getFirst()` as a method. stringmap has `first()` or `getKey()` as > methods :/). > happy to move out just the three of them into a helper like: > ``` > buildPostingList(StringMap<..>, TokenKind, Out); > ``` > if you think that's still helpful (that way we can also get rid of the calls > to clear since we can just move the maps into the function arguments). > yes that's the idea. would you prefer an explicit destructor call? I mean e.second.clear() in the loop. As it stands, you're only destroying them a whole map at a time, and trigrams is (i think) >50% of the total > the problem is 1 of these is a densemap, other three is a stringmap, and > onther is a vector. Sounds too complicated, nevermind! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124240/new/ https://reviews.llvm.org/D124240 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
