Page MenuHomePhabricator

[clangd] Adjust diagnostic range to be inside main file
ClosedPublic

Authored by kadircet on Thu, Jan 9, 8:04 AM.

Details

Summary

LSP requires diagnostics to lay inside main file. In clangd we keep
diagnostics in three different cases:

  • already in main file
  • adjusted to a header included in main file
  • has a note covering some range in main file

In the last case, we were not adjusting the diagnostics range to be in main
file, therefore these diagnostics ended up pointing some arbitrary locations.

This patch fixes that issue by adjusting the range of diagnostics to be the
first note inside main file when converting to LSP.

Diff Detail

Event Timeline

kadircet created this revision.Thu, Jan 9, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 9, 8:04 AM

Unit tests: pass. 61663 tests passed, 0 failed and 779 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ilya-biryukov accepted this revision.Thu, Jan 9, 8:47 AM

LGTM

clang-tools-extra/clangd/Diagnostics.cpp
343

NIT: arguably simpler with the standard algorithm

auto It = llvm::find_if(D.Notes, [](const Node&N) { return N.InsideMainFile; });
assert(It != D.Notes.end());

Up to you.

This revision is now accepted and ready to land.Thu, Jan 9, 8:47 AM
kadircet marked 2 inline comments as done.Thu, Jan 9, 9:00 AM
kadircet added inline comments.
clang-tools-extra/clangd/Diagnostics.cpp
343

agreed, thanks!

kadircet updated this revision to Diff 237105.Thu, Jan 9, 9:01 AM
kadircet marked an inline comment as done.
  • Use find_if instead of raw loop
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61663 tests passed, 0 failed and 779 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml