[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

[Sam McCall via cfe-commits]

On Sat, Sep 15, 2018, 08:36 Eric Liu via Phabricator <
reviews at reviews.llvm.org> wrote:

> ioeric added inline comments.
>
>
> ================
> Comment at: clangd/index/FileIndex.h:93
>  std::pair<SymbolSlab, RefSlab>
>  indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
> +         bool IndexMacros = false,
> ----------------
> sammccall wrote:
> > I'm concerned about the proliferation of parameters here. (Not new with
> this patch, it's been a continuous process - the first version had one
> input and one output!)
> > It's heading in the direction of being a catch-all "collect some data
> from an AST" function, at which point each caller has to think hard about
> every option and we're going to end up with bugs.
> > (For example: TestTU::index() passes "false" for IndexMacros. It seems
> to me maybe it should be "true". But it's really hard to tell.)
> > That's also pretty similar to the mission of SymbolCollector itself, so
> we're going to get some confusion there.
> >
> > As far as I can tell, there's now two types of indexing that we do:
> >   - preamble indexing: we look at all decls, and only index those
> outside the main file. We index macros. We don't collect references. Inputs
> are ASTContext and PP.
> >   - mainfile-style indexing: we look only at top-level decls in the main
> file. We don't index macros. We collect references from the main file.
> Inputs are ParsedAST.
> > This really looks like it should be 2 functions with 2 and 1 parameters,
> rather than 1 function with 4.
> > Then callers will have two styles of indexing (with names!) to choose
> between, rather than being required to design their own. And we can get rid
> of the "is this a main-file index?" hacks in the implementation.
> >
> > Sorry for jumping on you here, this change isn't any worse than the
> three previous patches that added knobs to this function.
> > This doesn't need to be addressed in this patch - could be before or
> after, and I'm happy to take this on myself. But I think we shouldn't kick
> this can down the road too much further, eventually we end up with APIs
> like clang :-)
> I'm definitely on board with this and happy to do the refactoring before
> landing this patch, to break API just once ;)  Just to clarify my
> understanding before doing that, do we also want to split
> `FileIndex::update` into `updatePreamble` and `updateMain`?  And the new
> `indexAST` will be `indexMain` and `indexPreamble`?
>
Basically yes, though it's a bit weird that each instance would use one
method or the other.

> I think if we're going to break the API, we should break it good :-)

FileIndex is pretty thin today logic-wise, all the opinions are in
FileSymbols, indexAST, and the DynamicIndex callers. I think it could stand
to take on more responsibility.
What if we merged DynamicIndex into it? So it'd have both updatePreamble
and updateMain, but they would update two separate FileSymbols. FileIndex
would maintain two SwapIndexes, and expose a merged index.

This would take the logic out of ClangdServer (where it's hard to test) and
also I think we have an out-of-tree DynamicIndex copy we could remove.
And I think the API would push us towards sensible decisions (e.g. I think
the answer for testTU::index() is it really wants to be a one-file
FileIndex with both indexing types)

Thoughts?


>
> Repository:
>   rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D52078
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180915/2a8e0e92/attachment.html>