thanks, looks better now. Some more comments on the test code.

Add the missing back newline.

Fix the comment.

Address review comments.

the change looks good, but I'd wait until D66219 is finished.

thanks.

avoid re-implementing of the method.

In D66211#1629112, @ilya-biryukov wrote:

How do they look in practice? In particular, should we add more context information (either in clangd or in the VSCode extension) to those errors, now that we know they are shown to the users?
Something like Failed to apply a fix: input range is invalid vs input range is invalid.

thanks, looks good.

thanks!

In D66074#1626932, @hans wrote:

thanks for the information, I suspect that this patch may depend on rL367303 (which is not merged in the release), I will double check.

Yes, that seems to be it. I've merged that, and then I could merge this one in r368683.

In D66074#1626856, @hans wrote:

In D66074#1624885, @hokein wrote:

@hans is there still any chance to merge this patch into the release?

I tried to merge this to release_90, but I got a test failure. Does this rely on more patches that need merging? Can you try applying it to the release branch in the monorepo and see what needs fixing?

$ ninja check-clangd
[7/8] Running the Clangd regression tests
llvm-lit: /work/llvm-9/llvm/utils/lit/lit/llvm/config.py:340: note: using clang: /work/llvm-9/build.release/bin/clang
FAIL: Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude (368 of 573)
******************** TEST 'Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude' FAILED ********************
Note: Google Test filter = IgnoreDiags.FromNonWrittenInclude
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from IgnoreDiags
[ RUN      ] IgnoreDiags.FromNonWrittenInclude
/work/llvm-9/llvm/tools/clang/tools/extra/clangd/unittests/DiagnosticsTests.cpp:945: Failure
Value of: TU.build().getDiagnostics()
Expected: is empty
  Actual: { [./a.h:0:0-0:4] 'main' must return 'int' }, which has 1 element
[  FAILED  ] IgnoreDiags.FromNonWrittenInclude (20 ms)
[----------] 1 test from IgnoreDiags (20 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (21 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] IgnoreDiags.FromNonWrittenInclude

 1 FAILED TEST
Preamble for file /clangd-test/TestTU.cpp cannot be reused. Attempting to rebuild it.
Built preamble of size 201076 for file /clangd-test/TestTU.cpp

********************
Testing Time: 3.92s
********************
Failing Tests (1):
    Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude

  Expected Passes    : 569
  Unsupported Tests  : 3
  Unexpected Failures: 1
FAILED: tools/clang/tools/extra/clangd/test/CMakeFiles/check-clangd

I think we could make the layering clearer:

mostly good with a few nits.

@hans is there still any chance to merge this patch into the release?

Add a comment.

Add a comment.

nice, just a nit.

looks good from my side.

please update the patch description.

Address comments.

mostly good, please update the patch description.

thanks, looks simpler now, just a few more nits.

Haven't looked at the patch in details.