This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add support for FixIts.
ClosedPublic

Authored by bkramer on Mar 1 2017, 6:34 AM.

Details

Summary

This uses CodeActions to show 'apply fix' actions when code actions are
requested for a location. The actions themselves make use of a
clangd.applyFix command which has to be implemented on the editor side. I
included an implementation for vscode.

This also adds a -run-synchronously flag which runs everything on the main
thread. This is useful for testing.

Event Timeline

bkramer created this revision.Mar 1 2017, 6:34 AM
krasimir added inline comments.Mar 1 2017, 7:39 AM
clangd/ASTManager.cpp
105

Why not typedef the type of the FixIts map?

124

I prefer this local variable to have a different name.

134

Consider the following situation:

  1. worker thread is at line 130 just before the critical section with full info about file A in LocalFixits; main thread takes the lock at line 159 in onDocumentAdd(file B)
  2. main thread finishes the critical section, calling CV.notify_one(), which has no effect and releases RequestLock.
  3. worker thread then gets RequestLock and enters the critical section at line 133, which outputs essentially stale diagnostics.

Arguably this seems OK, but maybe we want a separate mutex controlling access to the FixIts, which also might be more logical.
I can't find a real issue with the code as-is, you can leave it like this, I'm just wondering.

clangd/clients/clangd-vscode/src/extension.ts
26

Consider using const instead of let.

bkramer updated this revision to Diff 90181.Mar 1 2017, 8:07 AM
  • Use typedef instead of decltype()
  • Rename local variable not to shadow member.
  • Give FixIts their own mutex
  • Use const instead of let in typescript
krasimir accepted this revision.Mar 1 2017, 8:15 AM

Looks Great!

This revision is now accepted and ready to land.Mar 1 2017, 8:15 AM
This revision was automatically updated to reflect the committed changes.