- User Since
- Aug 26 2016, 6:53 AM (230 w, 8 h)
Add tests and fix bugs
Rebase, implement yaml bits (no tests yet)
Thanks, I'd seen these crashes but not manage to track down yet!
Wed, Jan 20
Mon, Jan 18
Thanks a lot for doing this!
I have some thoughts on how we can give it a nice API/more clearly separate it from FindTarget/improve internal structure.
Can you add a test for whatever this is fixing?
Thu, Jan 14
Wed, Jan 13
Thanks, this looks pretty solid!
Tue, Jan 12
In fact, never index the same file twice at all.
Thanks for looking into this feature!
(What broke this?)
Ah, this is a bit ugly... I'm not really happy with our representation of info extracted from the preprocessor, but that's something to tackle another time.
Mon, Jan 11
Thanks! This is a really good idea, but not without its risks :-) Sorry for not getting to this for a while!
Doh, I really thought we'd get away without an explicit recursion guard here.
But the example is compelling, so this seems like the right approach. Unfortunately, I think we're going to end up needing to add allocations too...
Fri, Jan 8
Yes, I was getting deja vu reading the description. Thanks though!
Thu, Jan 7
Thanks, this looks much better!
Tue, Jan 5
Ah, thanks for working on this!
I tried to sketch a couple of tetris-based mental models for why these hashtables might never fit inside an existing gap, leading us to stack our arena size (tetris field height) higher and higher.
This is predicated on the assumption that the index hashtables are the biggest single contiguous allocations in clangd, which I think is true. (It also explains why std::map fares better - each node is a separate allocation. Unfortunately, this is also why it's performance is terrible...)
Not sure which/either is model true, but I found it entertaining.
Seems fair enough to do something about this, though it's a bit sad we're doing it just because VSCode has bad UI.
Yup, as mentioned on D93393 this seems like a reasonable tradeoff to me. Thanks!
Sorry for the delay getting this landed, and thanks for the patch!
It'll be part of the clangd 12 release.
Sat, Dec 26
@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...
Dec 23 2020
Dec 22 2020
Thanks, this looks good, just a couple of nits.
Oops, I originally did plan to make operator() const, but then decided it'd be clearer for the caller to make PeriodicThrottler mutable instead. (Because pretending it doesn't have state is confusing).
Removed mutable from Next.
Dec 21 2020
This seems a bit too conservative to me, I wouldn't expect anyone to ever encounter such build problems (e.g. glibc so old that malloc_trim doesn't exist, or header exists but library doesn't), so I'd add a workaround if they actually arose.
The other value of the CMake option is being able to disable the *default* behavior if you use another allocator, so that you don't have to ask all users to pass a custom flag.
It's up to you - it's easy for me/others to land them on your behalf. Having your own commit access gives you more control, ability to deal with build breakages quickly, fix trivial things without review etc.
I've put a version of the reading part in D93653. This seems a bit less invasive to me - I'm still not sure if it's worth the readability hit though...
Thanks, this version seems just as clear as the original to me, and fewer allocations is nice :-)
Thanks, LG - want me to land it?
Thanks, this looks like a good idea to me, but some quibbles with the details:
- for any string, we statically know whether it's path or WD-relative
- we should make sure all the IO gets cached
Awesome! Some nits on naming and stuff, but the only real substantial question for me is the exact availability/behavior of the command-line flag. Even there, as long as the default behavior is to trim when build with glibc, the details are less important.
Let me know when you want me to land it!
Dec 20 2020
Sorry to mess you around like this, but I think I might have jumped the gun in asking this to be abstracted away in Process.h. My reasoning is:
- Once we are injecting the cleanup function from ClangdMain, that's not actually a crazy place to put this kind of system-level code, behind #ifdefs
- The abstraction costs us something: we can't skip all the locking etc for memory cleanups if it's a no-op on our platform
- We're putting an abstraction around one implementation without any reason to believe other implementations need this treatment or can use a similar interface. This is likely to turn out to be a poor abstraction, and llvm/Support isn't the easiest place to iterate on it.
Dec 18 2020
Thanks! Changes to ClangdServer are definitely good, I'm less sure about renderTUAction.
Thanks, this looks much closer to something we can land.
Bunch of nits :-)