[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet added a comment. In https://reviews.llvm.org/D51747#1233530, @ilya-biryukov wrote: > In https://reviews.llvm.org/D51747#1233499, @kadircet wrote: > > > Np, not planning to land it before we figure out the issue with vim can't > > handling lots of diagnostics anyway. > > > Just wondering: what's the problem with Vim? Slowness? Bad UI for diagnostics > when there are many of those? No, it starts to lag after some amount of diagnostics. I can't even move between lines in the file without a huge latency, and it doesn't return to normal after some time. 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet updated this revision to Diff 165462.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Add matchers to test.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -43,6 +43,11 @@
namespace {
+MATCHER(DeprecationWarning, "") {
+ return arg.Category == "Deprecations" &&
+ arg.Severity == DiagnosticsEngine::Warning;
+}
+
bool diagsContainErrors(const std::vector ) {
for (auto D : Diagnostics) {
if (D.Severity == DiagnosticsEngine::Error ||
@@ -963,6 +968,42 @@
Field(::Name, "baz")));
}
+TEST(ClangdCompilecommand, DiagnosticDeprecated) {
+ std::string Code(R"cpp(
+void foo() __attribute__((deprecated));
+void bar() {
+ foo();
+}
+ )cpp");
+ auto SourcePath = testPath("source/foo.cpp");
+
+ MockFSProvider FS;
+ struct DiagConsumer : public DiagnosticsConsumer {
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {
+ Diags.insert(Diags.end(), Diagnostics.begin(), Diagnostics.end());
+}
+
+void reset() {
+ Diags.clear();
+}
+
+std::vector Diags;
+ } DiagConsumer;
+ MockCompilationDatabase CDB;
+ CDB.ExtraClangFlags.push_back("-Wno-deprecated");
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ runAddDocument(Server, SourcePath, Code);
+ EXPECT_THAT(DiagConsumer.Diags, ElementsAre(DeprecationWarning()));
+ DiagConsumer.reset();
+
+ CDB.ExtraClangFlags.push_back("-Werror");
+ runAddDocument(Server, SourcePath, Code);
+ EXPECT_THAT(DiagConsumer.Diags, ElementsAre(DeprecationWarning()));
+}
+
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,15 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
+ // These flags are working for both gcc and clang-cl driver modes.
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ // Deprecations are often hidden for full-project build. They're useful in
+ // context.
+ C->CommandLine.push_back("-Wdeprecated");
+ // Adding -Wdeprecated would trigger errors in projects what set -Werror.
+ C->CommandLine.push_back("-Wno-error=deprecated");
return std::move(*C);
}
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -43,6 +43,11 @@
namespace {
+MATCHER(DeprecationWarning, "") {
+ return arg.Category == "Deprecations" &&
+ arg.Severity == DiagnosticsEngine::Warning;
+}
+
bool diagsContainErrors(const std::vector ) {
for (auto D : Diagnostics) {
if (D.Severity == DiagnosticsEngine::Error ||
@@ -963,6 +968,42 @@
Field(::Name, "baz")));
}
+TEST(ClangdCompilecommand, DiagnosticDeprecated) {
+ std::string Code(R"cpp(
+void foo() __attribute__((deprecated));
+void bar() {
+ foo();
+}
+ )cpp");
+ auto SourcePath = testPath("source/foo.cpp");
+
+ MockFSProvider FS;
+ struct DiagConsumer : public DiagnosticsConsumer {
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {
+ Diags.insert(Diags.end(), Diagnostics.begin(), Diagnostics.end());
+}
+
+void reset() {
+ Diags.clear();
+}
+
+std::vector Diags;
+ } DiagConsumer;
+ MockCompilationDatabase CDB;
+ CDB.ExtraClangFlags.push_back("-Wno-deprecated");
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ runAddDocument(Server, SourcePath, Code);
+ EXPECT_THAT(DiagConsumer.Diags, ElementsAre(DeprecationWarning()));
+ DiagConsumer.reset();
+
+ CDB.ExtraClangFlags.push_back("-Werror");
+ runAddDocument(Server, SourcePath, Code);
+ EXPECT_THAT(DiagConsumer.Diags, ElementsAre(DeprecationWarning()));
+}
+
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,15 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
+ // These flags are working for both gcc and clang-cl driver modes.
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ // Deprecations are
[PATCH] D51747: [clangd] Show deprecation diagnostics
sammccall accepted this revision.
sammccall added a comment.
Awesome, thank you.
Just a couple of last nits.
Comment at: unittests/clangd/ClangdTests.cpp:984
+std::vector Diagnostics) override {
+ std::lock_guard Lock(Mutex);
+ for(const Diag& D : Diagnostics) {
Locking is unneccesary. (Diagnostics will be delivered once, and the SyncAPI
calls block until diagnostics are delivered)
Comment at: unittests/clangd/ClangdTests.cpp:1014
+ runAddDocument(Server, SourcePath, Test.code());
+ EXPECT_TRUE(DiagConsumer.workedAsExpected());
+ DiagConsumer.reset();
nit: the logic is right, but the message could be better.
If we change something and it fails it will print `Expected true:
DiagConsumer.worksAsExpected(), but was false` or so. There are a number of
things that could be wrong.
Prefer just to capture a bit more data (all diagnostics) and use a matcher
expression:
```
MATCHER(DeprecationWarning, "") {
return arg.Category == "Deprecations" && arg.Severity ==
DiagnosticsEngine::Warning;
}
...
EXPECT_THAT(Diags, ElementsAre(DeprecationWarning()));
```
That way if there isn't exactly one diagnostic that's a deprecation warning,
it'll print the expectation, the full list of diagnostics, and the reason it
didn't match.
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
[PATCH] D51747: [clangd] Show deprecation diagnostics
ilya-biryukov added a comment. In https://reviews.llvm.org/D51747#1233499, @kadircet wrote: > Np, not planning to land it before we figure out the issue with vim can't > handling lots of diagnostics anyway. Just wondering: what's the problem with Vim? Slowness? Bad UI for diagnostics when there are many of those? 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet added a comment. In https://reviews.llvm.org/D51747#1233401, @sammccall wrote: > In https://reviews.llvm.org/D51747#1233377, @kadircet wrote: > > > In https://reviews.llvm.org/D51747#1233371, @sammccall wrote: > > > > > Sorry for all the back and forth on this patch, but I have to ask... > > > what's up with switching to lit tests? > > > > > > We've mostly started avoiding these as they're hard to maintain and debug > > > (not to mention write... crazy sed tricks!) > > > > > > They're mostly used in places where we need to test specifically the > > > protocol (smoke testing features, startup/shutdown, protocol edge > > > cases...) > > > > > > Sorry my bad actually forgot to mention. We set additional flags in > > ClangdServer::getCompileCommand and it is not used on the path of TestTU > > actually, instead it creates its own command line flags and uses them to > > invoke the compiler. So I changed to lit tests to make sure it gets invoked > > through getCompileCommand > > > Ah, that's unfortunate :-( > Sorry to be a pain, but can these be in ClangdTests instead? (Should really > be called ClangdServerTests). It's not as nice as TestTU, but... Np, not planning to land it before we figure out the issue with vim can't handling lots of diagnostics anyway. 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet updated this revision to Diff 165302.
kadircet added a comment.
This revision is now accepted and ready to land.
- Turn back to unit tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,63 @@
Field(::Name, "baz")));
}
+TEST(ClangdCompilecommand, DiagnosticDeprecated) {
+ Annotations Test(R"cpp(
+void foo() __attribute__(($foodeprecation[[deprecated]]));
+class A {
+public:
+ int x __attribute__(($xdeprecation[[deprecated]]));
+};
+int main() {
+ $foo[[foo]]();
+ return A().$x[[x]];
+}
+ )cpp");
+ auto SourcePath = testPath("source/foo.cpp");
+
+ MockFSProvider FS;
+ struct DiagConsumer : public DiagnosticsConsumer {
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {
+ std::lock_guard Lock(Mutex);
+ for(const Diag& D : Diagnostics) {
+if(D.Category == "Deprecations") {
+ HadDeprecation = true;
+ if (D.Severity == DiagnosticsEngine::Error ||
+ D.Severity == DiagnosticsEngine::Fatal)
+HadDeprecationAsError = true;
+}
+ }
+}
+
+bool workedAsExpected() {
+ return !HadDeprecationAsError && HadDeprecation;
+}
+
+void reset() {
+ HadDeprecation = false;
+ HadDeprecationAsError = false;
+}
+
+ private:
+std::mutex Mutex;
+bool HadDeprecationAsError = false;
+bool HadDeprecation = false;
+ } DiagConsumer;
+ MockCompilationDatabase CDB;
+ CDB.ExtraClangFlags.push_back("-Wno-deprecated");
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ runAddDocument(Server, SourcePath, Test.code());
+ EXPECT_TRUE(DiagConsumer.workedAsExpected());
+ DiagConsumer.reset();
+
+ CDB.ExtraClangFlags.push_back("-Werror");
+ runAddDocument(Server, SourcePath, Test.code());
+ EXPECT_TRUE(DiagConsumer.workedAsExpected());
+}
+
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,15 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
+ // These flags are working for both gcc and clang-cl driver modes.
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ // Deprecations are often hidden for full-project build. They're useful in
+ // context.
+ C->CommandLine.push_back("-Wdeprecated");
+ // Adding -Wdeprecated would trigger errors in projects what set -Werror.
+ C->CommandLine.push_back("-Wno-error=deprecated");
return std::move(*C);
}
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,63 @@
Field(::Name, "baz")));
}
+TEST(ClangdCompilecommand, DiagnosticDeprecated) {
+ Annotations Test(R"cpp(
+void foo() __attribute__(($foodeprecation[[deprecated]]));
+class A {
+public:
+ int x __attribute__(($xdeprecation[[deprecated]]));
+};
+int main() {
+ $foo[[foo]]();
+ return A().$x[[x]];
+}
+ )cpp");
+ auto SourcePath = testPath("source/foo.cpp");
+
+ MockFSProvider FS;
+ struct DiagConsumer : public DiagnosticsConsumer {
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {
+ std::lock_guard Lock(Mutex);
+ for(const Diag& D : Diagnostics) {
+if(D.Category == "Deprecations") {
+ HadDeprecation = true;
+ if (D.Severity == DiagnosticsEngine::Error ||
+ D.Severity == DiagnosticsEngine::Fatal)
+HadDeprecationAsError = true;
+}
+ }
+}
+
+bool workedAsExpected() {
+ return !HadDeprecationAsError && HadDeprecation;
+}
+
+void reset() {
+ HadDeprecation = false;
+ HadDeprecationAsError = false;
+}
+
+ private:
+std::mutex Mutex;
+bool HadDeprecationAsError = false;
+bool HadDeprecation = false;
+ } DiagConsumer;
+ MockCompilationDatabase CDB;
+ CDB.ExtraClangFlags.push_back("-Wno-deprecated");
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ runAddDocument(Server, SourcePath, Test.code());
+ EXPECT_TRUE(DiagConsumer.workedAsExpected());
+ DiagConsumer.reset();
+
+ CDB.ExtraClangFlags.push_back("-Werror");
+ runAddDocument(Server, SourcePath, Test.code());
+
[PATCH] D51747: [clangd] Show deprecation diagnostics
sammccall added a comment. In https://reviews.llvm.org/D51747#1233377, @kadircet wrote: > In https://reviews.llvm.org/D51747#1233371, @sammccall wrote: > > > Sorry for all the back and forth on this patch, but I have to ask... what's > > up with switching to lit tests? > > > > We've mostly started avoiding these as they're hard to maintain and debug > > (not to mention write... crazy sed tricks!) > > > > They're mostly used in places where we need to test specifically the > > protocol (smoke testing features, startup/shutdown, protocol edge cases...) > > > Sorry my bad actually forgot to mention. We set additional flags in > ClangdServer::getCompileCommand and it is not used on the path of TestTU > actually, instead it creates its own command line flags and uses them to > invoke the compiler. So I changed to lit tests to make sure it gets invoked > through getCompileCommand Ah, that's unfortunate :-( Sorry to be a pain, but can these be in ClangdTests instead? (Should really be called ClangdServerTests). It's not as nice as TestTU, but... 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet added a comment. In https://reviews.llvm.org/D51747#1233371, @sammccall wrote: > Sorry for all the back and forth on this patch, but I have to ask... what's > up with switching to lit tests? > > We've mostly started avoiding these as they're hard to maintain and debug > (not to mention write... crazy sed tricks!) > > They're mostly used in places where we need to test specifically the protocol > (smoke testing features, startup/shutdown, protocol edge cases...) Sorry my bad actually forgot to mention. We set additional flags in ClangdServer::getCompileCommand and it is not used on the path of TestTU actually, instead it creates its own command line flags and uses them to invoke the compiler. So I changed to lit tests to make sure it gets invoked through getCompileCommand 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
sammccall added a comment. Sorry for all the back and forth on this patch, but I have to ask... what's up with switching to lit tests? We've mostly started avoiding these as they're hard to maintain and debug (not to mention write... crazy sed tricks!) They're mostly used in places where we need to test specifically the protocol (smoke testing features, startup/shutdown, protocol edge cases...) 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet planned changes to this revision. kadircet added a comment. When doing some manual testing saw that it causes a lot of slow down on vim, even when I have just about 200 diagnostics, will wait till a few others check that out. We might wanna limit the number of diagnostics and keep only the ones that are closest to current cursor position. 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
[PATCH] D51747: [clangd] Show deprecation diagnostics
kadircet updated this revision to Diff 165275.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Convert unit tests to lit tests and address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
test/clangd/override-no-deprecations.test
Index: test/clangd/override-no-deprecations.test
===
--- /dev/null
+++ test/clangd/override-no-deprecations.test
@@ -0,0 +1,55 @@
+# Test that we return deprecation diagnostics even when -Wno-deprecations is
set.
+# And always return as warning even if -Werror is set.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -Wno-deprecated
the-file.cpp", "file": "../the-file.cpp"}]' >
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -Wno-deprecated
-Werror the-file.cpp", "file": "../the-file.cpp"}]' >
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"void
foo() __attribute__((deprecated));\nint main() { foo(); }"}}}
+# CHECK: "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "message": "'foo' is deprecated\n\nthe-file.cpp:1:27:
note: 'foo' has been explicitly marked deprecated here",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK: "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "message": "'foo' is deprecated\n\nthe-file.cpp:1:27:
note: 'foo' has been explicitly marked deprecated here",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK: "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "message": "'foo' is deprecated\n\nthe-file.cpp:1:27:
note: 'foo' has been explicitly marked deprecated here",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 16,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 13,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: "severity": 2
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,15 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
+ // These flags are working for both gcc and clang-cl driver modes.
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ // Deprecations are often hidden for full-project build. They're useful in
+ // context.
+ C->CommandLine.push_back("-Wdeprecated");
+ // Adding -Wdeprecated would trigger errors in projects what set -Werror.
+ C->CommandLine.push_back("-Wno-error=deprecated");
return std::move(*C);
}
Index: test/clangd/override-no-deprecations.test
===
--- /dev/null
+++ test/clangd/override-no-deprecations.test
@@ -0,0 +1,55 @@
+# Test that we return deprecation diagnostics even when -Wno-deprecations is set.
+# And always return as warning even if -Werror is set.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -Wno-deprecated the-file.cpp", "file": "../the-file.cpp"}]' >
[PATCH] D51747: [clangd] Show deprecation diagnostics
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
