<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 15, 2018, 08:36 Eric Liu via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ioeric added inline comments.<br>
<br>
<br>
================<br>
Comment at: clangd/index/FileIndex.h:93<br>
 std::pair<SymbolSlab, RefSlab><br>
 indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,<br>
+         bool IndexMacros = false,<br>
----------------<br>
sammccall wrote:<br>
> 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!)<br>
> 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.<br>
> (For example: TestTU::index() passes "false" for IndexMacros. It seems to me maybe it should be "true". But it's really hard to tell.)<br>
> That's also pretty similar to the mission of SymbolCollector itself, so we're going to get some confusion there.<br>
> <br>
> As far as I can tell, there's now two types of indexing that we do:<br>
>   - 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.<br>
>   - 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.<br>
> This really looks like it should be 2 functions with 2 and 1 parameters, rather than 1 function with 4.<br>
> 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.<br>
> <br>
> Sorry for jumping on you here, this change isn't any worse than the three previous patches that added knobs to this function.<br>
> 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 :-)<br>
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`?<br></blockquote></div></div><div dir="auto">Basically yes, though it's a bit weird that each instance would use one method or the other.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div><div dir="auto">I think if we're going to break the API, we should break it good :-)</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto">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)</div><div dir="auto"><br></div><div dir="auto">Thoughts?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Repository:<br>
  rCTE Clang Tools Extra<br>
<br>
<a href="https://reviews.llvm.org/D52078" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D52078</a><br>
<br>
<br>
<br>
</blockquote></div></div></div>