- User Since
- Feb 14 2018, 2:16 AM (153 w, 1 d)
LGTM, a friendly ping to @nridge though, as he was working on theia related stuff (at least in the past, not sure if you still do :D), to give him a heads-up if this is going to require some changes on their end.
Tue, Jan 19
Fri, Jan 15
btw, do you have commit access or should i land this for you ? (if so please provide your email)
thanks for the fix, a small comment about testing though.
Wed, Jan 13
thank, as discussed offline this looks like the right thing to do before the branch cut. we might re-evaluate our decision around re-indexing on cmd changes and canonicalization one day ...
Tue, Jan 12
Ah this is really unfortunate :( In theory we already know which slabs belong to main file while updating the index for the preamble, as we shard them per file. But it is really inconvenient layering wise to transfer that slab from updatePreamble to updateMain in parsing callbacks :/
Another layer to fix the issue would be to somehow make main file parsing, re-parse those macro definitions through replay mechanism or preamble patching, but these might have unwanted consequences as we would see a re-definition of the macro now. Also they are considerably hard :/
Sun, Jan 10
oh and also thanks a lot for all the investigation and fix of the issue!
Fri, Jan 8
(not sure if you were looking for comments yet, but i was just passing by and it was a small-ish patch, so couldn't resist :D)
Thu, Jan 7
Tue, Jan 5
i thought clang-format was also putting angled includes and "OtherHeaders" into same category, by looking at:
Dec 9 2020
this mostly LGTM. there are some changes to the existing behavior; like discovery order and not looking for other plugins under build/, but they seem like non-harmful changes to me.
Dec 1 2020
can you give me an email address to associate the commit with?
Nov 27 2020
Do you have commit access or should I commit this for you?
Haven't checked the details but is there a specific reason for implementing a custom protocol rather than making use of NotifyOnStateChange (https://grpc.github.io/grpc/cpp/classgrpc_1_1_channel_interface.html) or even WaitForStateChange if we really want to block the channel creation ?
Nov 26 2020
Nov 25 2020
nope, as it doesn't change the serialization format. but any existing shards won't have refs from the main file, they'll accumulate over time as sources gets modified. (i don't think it is worth bumping the version to invalidate existing shards, if a user cares enough they can manually delete their cache)
Thanks for the explanations. I've also had some discussions with @sammccall about this and would like to summarize them.
Nov 24 2020
Thanks, this sounds like a sensible idea. I got a few suggestions for the implementation though.
- Link protobuf and grpc++ publicly to generated targets
NVM, found the summary https://github.com/clangd/clangd/issues/162#issuecomment-653981038
i remember discussing this in the past, and having "negative" feelings about it. I couldn't find the discussion on the issues page tho, could you toss me the link if you can find it? (it might've been an offline discussion as well, sorry if that's the case for not dumping a summary).
Nov 23 2020
Thanks a lot for working on improving clangd!
This looks great! Thanks a lot for bearing with me and doing all of this!
Nov 22 2020
- Address comments
Nov 21 2020
Merged with https://reviews.llvm.org/D91860
- Merge with https://reviews.llvm.org/D90751
- Address comments
I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.
- Address comments
Nov 20 2020
This looks like an improvement to me as well, thanks!
- Mention assertions and ninja
- Internalize synchronization of indexstorage rather than using Memoize