[PATCH] D51747: [clangd] Show deprecation diagnostics

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/ClangdServer.cpp:534
 
-  // Inject the resource dir.
+  // Inject the resource dir. These flags are working for both gcc and clang-cl
+  // driver modes.
----------------
nit: the "these flags" is not just for resource dir, can you move that second sentence onto a line above?


================
Comment at: clangd/ClangdServer.cpp:538
   C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+  C->CommandLine.push_back("-Wdeprecated");
+  C->CommandLine.push_back("-Wno-error=deprecated");
----------------
each of these lines deserves a comment I think.
e.g.
`// Deprecations are often hidden for full-project build. They're useful in context.`
-Wdeprecated
`// Adding -Wdeprecated would trigger errors in projects what set -Werror.`
-Wno-error=deprecated


================
Comment at: unittests/clangd/ClangdUnitTests.cpp:223
 
+TEST(DiagnosticsTest, DiagnosticDeprecated) {
+  Annotations Test(R"cpp(
----------------
I think you should set TestTU.ExtraArgs to "-Wno-deprecated" (with a comment) to verify it's overridden.
Both for clarity, and because I think -Wdeprecated is actually on by default, so I think this test as written would pass without your change.


================
Comment at: unittests/clangd/ClangdUnitTests.cpp:248
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+  Annotations Test(R"cpp(
----------------
I'm not sure exactly what this is testing - I think it's a combination of features that are tested elsewhere. Feel free to drop.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51747