- User Since
- Apr 6 2017, 1:42 AM (70 w, 5 d)
- run clang-format
There's a test for the new behavior in D50726
LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.
Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting with failure error code
LGTM. Many thanks for fixing this.
Sorry for missing this one. The biggest concern that I have is about the thread-safety of the API, it's too easy to misuse from multiple threads at this point.
- Use 'auto*' instead of 'auto'
Maybe also add a test for find-definition that was broken before? (non-empty preamble -> empty preamble -> bad gotodef that goes to included file instead of the local variable)
To have a regression test against similar failures.
Fri, Aug 10
NIT: maybe also note the number of the clangd revision in this change's description
LGTM, but let's land together with a dependent revision to hove some code that actually tests it.
Thanks! LGTM with a few NITs
Sure, LGTM, did not want to block you.
LGTM. Thanks for the change!
Could we add an option to clangd to switch it on? (VSCode does not work, but our hacked-up ycm integration seems to work, right?)
LGTM with a small NIT.
Only a few NITs from my side.
Excited for this fix to get in, have been seeing duplicate in other cases too :-)
I have left a few comments, but wanted to start with a higher-level design consideration.
Thu, Aug 9
Maybe hard-code the SHA256 checksum directly into the script instead?
Going through the keyservers does not seem to buy us much in terms of security and we certainly don't update the Dockerfiles very often, so it does not take too much time to verify those SHA256 checksums by hand when we do.
Using the gpg was a mistake on my end, it makes things more complicated and less reliable.
The new behavior looks reasonable for interactive tools like clangd, but I'm a bit worried that clang may emit too many irrelevant errors, because it may not cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that mode.
Nevertheless, I think this is moving clangd in the right direction and if we see too many irrelevant errors, we should look into improving clang to show less of those.
Wed, Aug 8
Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we should try to avoid using it where possible.
On the other hand, where this key is stored might not be terribly important if we don't have too many usages and remove it when we can workaround the limitations that we're currently facing.
Maybe split the commit message into multiple lines?
The presentation of diagnostics to the users is different between clang and clangd:
- clang diagnostics are always prefixed with the file, location and severity of the diagnostic, e.g. main.cpp:1:2: error: expected '}'
- In LSP clients (VSCode in particular), the diagnostic message is the first thing the user sees and it looks a little nicer (subjectively) if the first letter is capitalized.
+1 Sam's suggestion of configuring this during initial LSP handshake, rather than as a command-line flag.
We could put it into ClientCapabilities or into initializationOptions. The latter has type 'any' in LSP, so it seems to be most in-line with the protocol to put clangd-specific config options there, but not opposed to having this in other structs as an extension either.
Tue, Aug 7
LGTM, with a few NITs.
Thanks for the change!
Thanks! The only major comment I have is about the FixItsScore, others are mostly NITs.
Mon, Aug 6
Fri, Aug 3
This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?
Thanks for the change! Overall LG, mostly NITs about the tests.
Thu, Aug 2
Seems we have consensus here.
Let's remove the option and just always capitalize the messages.
Wed, Aug 1
Picking it up since @sammccall is OOO.
Abandoning in favor of George's fix.
LGTM with a small nit. Thanks for the change!
Tue, Jul 31
This change is a bit ugly since it has to account for the value of LLVM_ENABLE_THREADS...
Happy to accept suggestions/alternative ways to fix this :-)
Are you referring to the file-locking on Windows?
Any other reasons why the -isystem trick might be non-ideal?
- Rebase onto head
- Check for PrevDiagsWereReported
- Moved early return into the CanReuseAST branch
- Added a comment
Sorry for the delay, somehow missed this update in my inbox.
Just a few nits left.
Mon, Jul 30
Sorry for sending this in instead of waiting for D49158 to land, I hadn't noticed it earlier
Fri, Jul 27
- Move the test into an existing file
Thu, Jul 26
Many thanks! Great cleanup. Just a few nits and we're good to go
- Move OldPreamble.reset() out of the lock, add a comment
Not strictly opposed to this change, but should any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?
Wed, Jul 25
- Use log instead of vlog
- Add parens around &&
Overall LG. I'm sure it's an improvement overall, just wanted to get some clarifying comments and references, if that's possible.
For the record: this got reverted in rL337850
Tue, Jul 24
The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?
if/when we have a native implementation, supporting multiple mechanisms with different layering requirements to get at most a 2x win in watcher resource usage seems like a dubious way to spend our complexity budget.
I guess the only think that might force us to do this is the low system limit on the number of file watches. I do remember rumors that it can be a problem in, but I haven't really seen those on my own, so maybe I'm just pretending.
But I do agree that we shouldn't commit to have yet-another implementation in the long-term, unless we have good justification for it.
Yeah, sounds reasonable. The limits on the number of watches is definitely something that could bite the editor+clangd bundle, so that's a good reason to use the client watching if that's available.
We want to share some design around this area in Aug-Sep.
Yes, language client, thanks!
Wow, that's a huge oversight on my end.
Thanks for fixing this! LGTM