- User Since
- Aug 26 2016, 6:53 AM (119 w, 6 d)
Tue, Nov 27
Only really significant thing is I think the name "build graph" is confusing, and we should specifically mention includes.
Rest is just nits to take or leave...
Mostly just places that should be updated HintPath -> TUPath. Changing the whole codebase doesn't seem important, but in places that are touched...
To summarize offline discussion: I think we need to split this patch up, as the scope is growing. (In a really useful direction I think!)
Tests look great, thanks!
structure and serialization stuff looks great!
still some questions around the auto-index logic and organization of the extraction code, let's chat offline. May make sense to split.
Mon, Nov 26
Make blockUntilIdleForTest() accept a timeout, update comment.
You may want to add a FIXME in SymbolIndex to include opaque type in fuzzyfind request.
LG from my side.
If you have unit tests in the next couple of days, happy to take a look, otherwise go ahead once Alex/Ben are happy.
If the CDB dir is unknown, don't try to write shards to disk.
Fri, Nov 23
Thanks, this looks good! Just nits, and please do port most of the test cases to unit tests.
This is great!
Sorry, this was a mistake - we do in fact use decls that don't have symbol IDs (we just don't look them up in the index).
So I think both SymbolID and USR are optional.
Thu, Nov 22
Everything looks so much better...
Going to leave awkward comments suggesting we expand the scope a bit.
Full disclosure: file dependency information is something that's come up as useful in lots of contexts.
Just the context thing. Rest is some optional simplifications.
Wed, Nov 21
Thanks for sending this! Broadly looks good, a few details to work out. I think the biggest one is multiple symbols which you've flagged.
Tue, Nov 20
Very nice! Mostly just a few style/structure nits.
The biggest suggestion I have is merging the callback into ASTCallbacks. That's awkward in initialization (makeUpdateCallbacks probably needs to be called unconditionally with a maybe-null pointer) but it seems like a natural grouping in all the places it gets plumbed around to... and there are a lot!
Mon, Nov 19
Thanks! Sorry for missing this.
Address comments, add test for Event machinery.
Address review comments.
Fri, Nov 16
Thu, Nov 15
Remove clang-tidy changes, add FIXME comment.
Moved the clang-tidy changes to D54579.
Sorry for mixing everything up!
Address comments from the last round of review in D54204.
Thanks! Looks good, rest of the nits are obvious or up to you.(
As mentioned offline - sounds like someone from Apple might take a look.
If not, ping me again to land!