Page MenuHomePhabricator

[clangd] Emit ranges for clangd diagnostics, and fix off-by-one positions
ClosedPublic

Authored by sammccall on Dec 12 2017, 9:20 AM.

Details

Summary
  • when the diagnostic has an explicit range, we prefer that
  • if the diagnostic has a fixit, its RemoveRange is our next choice
  • otherwise we try to expand the diagnostic location into a whole token. (inspired by VSCode, which does this client-side when given an empty range)
  • if all else fails, we return the zero-width range as now. (clients react in different ways to this, highlighting a token or a char)
  • this includes the off-by-one fix from D40860, and borrows heavily from it

Diff Detail

Event Timeline

sammccall created this revision.Dec 12 2017, 9:20 AM
rwols accepted this revision.Dec 12 2017, 11:48 AM

LGTM, you can go ahead and land this, I'll drop D40860. I'm happy we got to the bottom of this :)

This revision is now accepted and ready to land.Dec 12 2017, 11:48 AM
This revision was automatically updated to reflect the committed changes.
hokein added inline comments.Dec 13 2017, 1:44 AM
test/clangd/diagnostics.test
10

Maybe the current tests are enough. Do we want to add a test where the diagnostic location is at the end of the line?

f()
// ^ missing ";"
f();
sammccall added inline comments.Dec 13 2017, 2:37 AM
test/clangd/diagnostics.test
10

Good point - the current tests only incidentally cover much of the diagnostics functionality.
The lit tests for this are brittle and hard to read, I'll write some gtest cases similar to CodeComplete.