hokein added inline comments.

================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:709
   ns::X *x;
+  auto& $type[[[]]a] = *x;
   x$access[[->]]f();
----------------
kadircet wrote:
> should this be `$type[[a]]` ?
yeah, but this is the actual diagnostic range, though it is wired. 


================
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(
----------------
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. 



================
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"),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88964

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to