[PATCH] D88964: [clangd] Add a missing include-fixer test for incomplete_type, NFC.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

still LGTM



================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:715
   auto TU = TestTU::withCode(Test.code());
+  TU.ExtraArgs.push_back("-std=c++17");
   auto Index = buildIndexWithSymbol(
----------------
hokein wrote:
> kadircet wrote:
> > why do we need c++17 ?
> The newly-added test case is using C++17 feature (structure binding), needs this flag to suppress a diagnostic warning. 
> 
ah okay i see it now, structured binding wasn't obvious from the test case above (i thought there was a typo :D


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:738
+                            "Add include \"x.h\" for symbol ns::X"))),
+          AllOf(Diag(Test.range("nested"),
+                     "incomplete type 'ns::X' named in nested name specifier"),
----------------
hokein wrote:
> kadircet wrote:
> > can you move this back to original position ?
> yeah, I reordered the list.
> 
> there are two orders: the one listed here, and the one listed in `IncludeFixer.cpp`.
> 
> The motivation is to make them aligned, so that it would be easier to compare and spot the difference. The current state is not friendly to readers.
i don't think the orderings in the source vs test is that relevant, but SGTM if it helps you. mainly because:
- as it might be harder in future to trigger diagnostics in the order we want
- it is an invariant hard to maintain without at least the reviewer or author of changes knowing about(and remembering) it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88964/new/

https://reviews.llvm.org/D88964