Page MenuHomePhabricator

[clangd] Drop diags from non-written #include.
ClosedPublic

Authored by hokein on Aug 12 2019, 1:45 AM.

Event Timeline

hokein created this revision.Aug 12 2019, 1:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2019, 1:45 AM
ilya-biryukov accepted this revision.Aug 12 2019, 1:59 AM

LGTM, thanks!
We should also merge this into the release branch if it's not too late yet.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
952

Could you add a comment describing which diagnostic we do not want to see here and why?
I assume it's main should return int, right?

This revision is now accepted and ready to land.Aug 12 2019, 1:59 AM
hokein updated this revision to Diff 214590.Aug 12 2019, 2:30 AM
hokein marked an inline comment as done.

Add a comment.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2019, 2:37 AM
hokein marked an inline comment as done.Aug 12 2019, 2:38 AM
hokein added a subscriber: hans.

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

hans added a comment.Aug 13 2019, 4:38 AM

@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

@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

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

hans added a comment.Aug 13 2019, 5:38 AM

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.

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.

Great, thanks!