- User Since
- Aug 26 2016, 6:53 AM (241 w, 1 d)
Fri, Apr 9
Using server: None for this seems a bit weird and irregular (why not file: none?)
Thu, Apr 8
Tue, Apr 6
(Do we have a bug this fixes?)
Mon, Apr 5
This is looking pretty good! Can't wait to try it, and some of my suggestions are just scope creep, so feel free to draw the line :-)
Wed, Mar 31
Thanks for working on this! It looks like it's getting traction in VSCode/LSP now, which makes it much more compelling.
Thu, Mar 25
Just doc nits - I think maybe there's some confusion on what a token range is.
Code looks good though!
Wed, Mar 24
Yup. Let's not bite off more for now.
- make ASTHooks own it and instantiate a new one on every parse (i think the cleanest and most explicit)
Agreed. (And this only one that overlaps this patch a lot)
Thanks for digging!
Tue, Mar 23
Thanks! Just some random nits/ideas.
Thanks for finding and working on this hotspot. (I'm curious *why* isBeforeInTranslationUnit is slow if you have any insight - it has sad worst-case but I thought we'd hit the one-element cache often enough in practice).
The fix doesn't look obviously correct: the side effect of marking the destructor reference seems important if we actually generate code. It's not obvious to me why the type can only be incomplete if there are errors.
Wed, Mar 17
Sorry about the lack of response here.
Tue, Mar 16
Mon, Mar 15
This seems like something we can delay until we're sure it'll do what we want? (e.g. we see that we either usually know synchronously whether lazy fixes will be present, or can predict it with high confidence)
Is this entirely obsoleted by the approach in D98499?
Should we have a test somewhere that tweaks defined in modules actually work?
I'm getting a little nervous about the amount of stuff we're packing into modules without in-tree examples.
I should split out some of the "standard" features into modules as that's possible already.
Thanks, nice to see this isn't going to require deep changes, and looking forward to seeing the diagnostics performance improvements.
(I wonder if we'll end up in the perverse situation where clangd is faster for most operations when your preamble is stale :-D Probably not...)
Fri, Mar 12
oops, enable clangd test
Mar 12 2021
Omit narrower, now-redundant check
Mar 11 2021
I've landed the caching patch so this is good to go.
The merge is *conceptually* trivial but the enclosing function has moved so you may need to reapply it by hand, sorry...
Mar 10 2021
Avoiding copies seems nice, but this makes some interfaces more awkward. Do you have any measurements that some of these copies matter?
Stop reporting offset for fields in a union, too
I measured this as a mild (3%) increase in preamble indexing time when working on clangd itself, more details in D98329
Some performance measurements...
(workload is PreambleCallback for clangd/XRefs.cpp in sync mode with a fixed baseline of 99b01cf28db9db1a3ec0e25367bd325b7aca6d43, opt build without asserts)
Mar 9 2021
LGTM with the doc nits from njames93 (and the doc generator script re-run)
I think this is going to be more good than bad, and the alternatives are terrifically complicated.
Still LG btw!
Thanks for working this out!
This looks pretty good, great cleanup!
Mar 8 2021
Nice fixes! A few drive-by comments from me, up to you though.
Also fixes a bug where the canonical delimiter was not applied for raw strings with empty delimiters detected via well-known enclosing functions that expect a text proto.
Mar 5 2021
Nice! Only substantive suggestion: I think requiring the base type to be exactly a standard map type is too conservative.
Both side-effects seem fine to me.
Mar 4 2021
I know we talked about names a bunch and i'm being a bit intransigent here.
Happy to go with Component if you think it's a better name, but wanted to see how this looks.
Mar 3 2021
Will the fixes/actions this enables really be available synchronously?
Mar 2 2021
fix bug link
OK, I think we've established that the request was for a way to dump out clangd's whole index, and workspace/symbol isn't actually a good way to do that.
Nevertheless, this seems useful and endorsed by the spec.
remove one more redundant getDraft() check
I've asked on the bug for some more context for the request.
I don't see a problem with this change per se but I'm not sure it'll actually fix the bug.
LGTM (apart from Aaron's comment!)