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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
