[PATCH] D52016: [clangd] Don't create child AND and OR iterators with one posting list

[Ilya Biryukov via Phabricator via cfe-commits]

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