ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM with a nit.
================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:148
+ if (TrigramIterators.size() > 1)
TopLevelChildren.push_back(createAnd(move(TrigramIterators)));
+ else if (TrigramIterators.size() == 1)
----------------
kbobyrev wrote:
> ilya-biryukov wrote:
> > Maybe special-case a single-iterator case in `createAnd` instead? Same with
> > `createOr`.
> > Any cons to doing so?
> I thought that this might result in some implicit query tree generation
> semantics. However, since there optimizations are likely to be added at some
> point, I guess we could start being implicit right now, especially given that
> there is no good way for the user code to inspect the query tree structure
> (and there's no good reason to do so, too).
As long as observable behaviour does not change (and this change does not seem
to change it), we should be good.
Creating optimized trees on-the-fly will make sure we always get optimal
results, which is nice.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:93
: Children(std::move(AllChildren)) {
- assert(!Children.empty() && "AND iterator should have at least one
child.");
// Establish invariants.
----------------
Maybe keep the assertion here?
It's clear that `createAnd` is the only place that creates this class, so we
can always trace the assertion back to its root-cause. Having the assertion in
ctor guards against possible modifications of the code that would add more ways
to construct `AndIterator`
Same for the assertion in `OrIterator`
https://reviews.llvm.org/D52016
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
