Advertisement

Close

Using server: None for this seems a bit weird and irregular (why not file: none?)

Advertisement

Close

(Do we have a bug this fixes?)

Advertisement

Close

Still LG

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 :-)

Thanks for working on this! It looks like it's getting traction in VSCode/LSP now, which makes it much more compelling.

Just doc nits - I think maybe there's some confusion on what a token range is.
Code looks good though!

In D98499#2644313, @kadircet wrote:

In D98499#2626502, @sammccall wrote:

My model for modules using this diagnostic stuff (apart from the build-system stuff which sadly can't be meaningfully upstreamed) are IncludeFixer, ClangTidy, and IWYU - worth thinking about how we'd build those on top of this model. (Doesn't mean we need to add a hook to emit diagnostics, but it probably means we should know where it would go)

Agreed. I believe they all would need extra end points to ASTHooks though.

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!

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.

Sorry about the lack of response here.

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.

In D98618#2625675, @njames93 wrote:

Can I ask what is the purpose of --skip-headers?
What does it offer that --header-filter doesn't already offer?

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...)

oops, enable clangd test

Omit narrower, now-redundant check

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...

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)

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!

Thanks, LG!

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.

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.

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.

In D96245#2602896, @kadircet wrote:

Agreed, I was going this way because currently there was no way to provide a code action to clangd::diagnostic apart from storing it in the fixes map while generating it. I suppose it is more sensible to just go with a new hook on modules that can provide a codeaction given a clangd::diagnostic.

Will the fixes/actions this enables really be available synchronously?

simplify

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.

In D97738#2597296, @kadircet wrote:

as discussed offline, i am hesitant about validateEdits layering at the moment. but i suppose the best thing to do is move it into clangdserver and expose a structrual api, as you proposed, which can be done independently.

address comments
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!)