This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Remove LSP command-based #include insertion.
ClosedPublic

Authored by ioeric on May 10 2018, 12:20 AM.

Details

Summary

clangd will populate #include insertions as addtionalEdits in completion items.

The code completion tests in ClangdServerTest will be added back in D46497.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.May 10 2018, 12:20 AM

This LG, but we should first roll out the additionalEdits change.
Aren't the dependencies reversed in the dependency stack, i.e. this change should be the last one?

This LG, but we should first roll out the additionalEdits change.
Aren't the dependencies reversed in the dependency stack, i.e. this change should be the last one?

You are right ;) I got the dependency reversed. My plan was to land this and the other patch at the "same" time.

ilya-biryukov added inline comments.May 11 2018, 6:57 AM
unittests/clangd/ClangdTests.cpp
941 ↗(On Diff #146078)

Do we test the same thing somewhere else (e.g. code completion) in one of the dependent changes?
Maybe it's worth noting in the commit description that this test was not removed completely, but instead moved to <new-test-name-here>?

ioeric edited the summary of this revision. (Show Details)May 11 2018, 7:03 AM
ioeric marked an inline comment as done.
ioeric added inline comments.
unittests/clangd/ClangdTests.cpp
941 ↗(On Diff #146078)

Sure thing.

This is tested in code completion unit tests in D46497. Noted this in patch summary.

ilya-biryukov accepted this revision.May 11 2018, 7:10 AM

LG when all the dependencies are done

unittests/clangd/ClangdTests.cpp
941 ↗(On Diff #146078)

Thanks!

This revision is now accepted and ready to land.May 11 2018, 7:10 AM
This revision was automatically updated to reflect the committed changes.
ioeric marked an inline comment as done.
clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test