kbobyrev planned changes to this revision. kbobyrev added a comment. In D84811#2199891 <https://reviews.llvm.org/D84811#2199891>, @hokein wrote:
> In D84811#2199829 <https://reviews.llvm.org/D84811#2199829>, @kbobyrev wrote: > >> In D84811#2199820 <https://reviews.llvm.org/D84811#2199820>, @hokein wrote: >> >>> In D84811#2199510 <https://reviews.llvm.org/D84811#2199510>, @kbobyrev >>> wrote: >>> >>>> Even despite `FileFilter` not being fully implemented yet (an issue for a >>>> separate patch) I think this change should still be correct and is >>>> probably OK to land, WDYT @hokein? >>> >>> Yes, personally no objection on landing this, but landing it doesn't seem >>> to help much for your remote-index case (STL symbols are not filtered out)? >> >> True, it doesn't filter _all_ of them, but it partially filters some symbols >> which is still a win I guess :) My rationale is probably that having this >> blocked on more implementation details would be more reasonable if there was >> no implementation at all but since it still filters out some symbols this >> should probably be fine. > > It just reduces symptoms, not solving the problem. I think remote-index is > also blocked the FileFiltering stuff (we can't launch remote-index without > fixing this issue entirely). > > As you may have noticed implementing FileFiltering is tricky, I think the > indexer here is a good opportunity to test/verify your implementation > (comparing the index data before vs after), so my preference would be to > implement the FileFilter, test it with this patch together, then finally land > this patch after making sure everything works. Fair enough, this is a viable strategy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84811/new/ https://reviews.llvm.org/D84811 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
