[PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.
kadircet added inline comments.
Comment at: clangd/ClangdServer.cpp:537
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated-declarations");
return std::move(*C);
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > as noted above I think we should also have
> > > -Wno-error=deprecated-declarations
> > >
> > > (do you want all of -Wdeprecated, actually?)
> > Yes for the second part and no for the first part. As we saw there are some
> > configs out there that treat some types of deprecation warnings as errors,
> > so we don't want to change that behavior.
> Doesn't this cause problems when the build flags are `-Wno-deprecated
> -Werror` then?
> We make this `-Wno-deprecated -Werror -Wdeprecated`, and now all uses of
> deprecated APIs are errors.
>
> This seems like a common configuration, highly noticeable symptoms, and not
> what the user wants...
Well, adding -Wno-error=deprecated to get over this case. Now we might have
problems if people wanted to see deprecation warnings as errors, but I believe
this shouldn't be much of a problem since they will get those errors at built
anytime and we won't be annoying other users(ones that show deprecation as
warnings or not showing them at all, hopefully almost everyone?) too much.
WDYT?
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] Implement deprecation diagnostics with lower severity.
kadircet updated this revision to Diff 165266.
kadircet marked an inline comment as done.
kadircet added a comment.
- Do not show up as errors even on codebases with -Werror.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -220,6 +220,44 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated"),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllOf(Diag(Test.range("x"), "'x' is deprecated"),
+WithNote(
+Diag(Test.range("xdeprecation"),
+ "'x' has been explicitly marked deprecated here");
+}
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+ Annotations Test(R"cpp(
+void bar();
+void foo() __attribute__((deprecated("", "bar")));
+int main() {
+ $deprecated[[foo]]();
+}
+ )cpp");
+ EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(WithFix(Fix(Test.range("deprecated"), "bar",
+ "change 'foo' to 'bar'";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,12 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
- // Inject the resource dir.
+ // Inject the resource dir. These flags are working for both gcc and clang-cl
+ // driver modes.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated");
+ C->CommandLine.push_back("-Wno-error=deprecated");
return std::move(*C);
}
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -220,6 +220,44 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated"),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllOf(Diag(Test.range("x"), "'x' is deprecated"),
+WithNote(
+Diag(Test.range("xdeprecation"),
+ "'x' has been explicitly marked deprecated here");
+}
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+ Annotations Test(R"cpp(
+void bar();
+void foo() __attribute__((deprecated("", "bar")));
+int main() {
+ $deprecated[[foo]]();
+}
+ )cpp");
+ EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(WithFix(Fix(Test.range("deprecated"), "bar",
+ "change 'foo' to 'bar'";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,12 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
- // Inject the resource dir.
+ // Inject the resource dir. These flags are working for both gcc and clang-cl
+ // driver modes.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated");
+ C->CommandLine.push_back("-Wno-error=deprecated");
return std::move(*C);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.
sammccall added inline comments.
Comment at: clangd/ClangdServer.cpp:537
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated-declarations");
return std::move(*C);
kadircet wrote:
> sammccall wrote:
> > as noted above I think we should also have
> > -Wno-error=deprecated-declarations
> >
> > (do you want all of -Wdeprecated, actually?)
> Yes for the second part and no for the first part. As we saw there are some
> configs out there that treat some types of deprecation warnings as errors, so
> we don't want to change that behavior.
Doesn't this cause problems when the build flags are `-Wno-deprecated -Werror`
then?
We make this `-Wno-deprecated -Werror -Wdeprecated`, and now all uses of
deprecated APIs are errors.
This seems like a common configuration, highly noticeable symptoms, and not
what the user wants...
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] Implement deprecation diagnostics with lower severity.
kadircet updated this revision to Diff 165252.
kadircet marked 5 inline comments as done.
kadircet added a comment.
- Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -220,6 +220,44 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated"),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllOf(Diag(Test.range("x"), "'x' is deprecated"),
+WithNote(
+Diag(Test.range("xdeprecation"),
+ "'x' has been explicitly marked deprecated here");
+}
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+ Annotations Test(R"cpp(
+void bar();
+void foo() __attribute__((deprecated("", "bar")));
+int main() {
+ $deprecated[[foo]]();
+}
+ )cpp");
+ EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(WithFix(Fix(Test.range("deprecated"), "bar",
+ "change 'foo' to 'bar'";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,11 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
- // Inject the resource dir.
+ // Inject the resource dir. These flags are working for both gcc and clang-cl
+ // driver modes.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated");
return std::move(*C);
}
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -220,6 +220,44 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated"),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllOf(Diag(Test.range("x"), "'x' is deprecated"),
+WithNote(
+Diag(Test.range("xdeprecation"),
+ "'x' has been explicitly marked deprecated here");
+}
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+ Annotations Test(R"cpp(
+void bar();
+void foo() __attribute__((deprecated("", "bar")));
+int main() {
+ $deprecated[[foo]]();
+}
+ )cpp");
+ EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(WithFix(Fix(Test.range("deprecated"), "bar",
+ "change 'foo' to 'bar'";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -531,9 +531,11 @@
if (!C) // FIXME: Suppress diagnostics? Let the user know?
C = CDB.getFallbackCommand(File);
- // Inject the resource dir.
+ // Inject the resource dir. These flags are working for both gcc and clang-cl
+ // driver modes.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated");
return std::move(*C);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.
kadircet added a comment.
As discussed offline with Ilya, decided to keep the compile flag addition since
it would be easier to pass the logic of a command line flag to that point. Also
not changing the severity level, since they will show up on diagnostics lists
in anyway it doesn't save much.
Comment at: clangd/ClangdServer.cpp:537
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated-declarations");
return std::move(*C);
sammccall wrote:
> as noted above I think we should also have -Wno-error=deprecated-declarations
>
> (do you want all of -Wdeprecated, actually?)
Yes for the second part and no for the first part. As we saw there are some
configs out there that treat some types of deprecation warnings as errors, so
we don't want to change that behavior.
Comment at: clangd/Diagnostics.cpp:299
+D.Severity =
+D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel;
return D;
sammccall wrote:
> not sure what the concrete benefits are from using Note rather than Warning.
> It's semantically iffy, so if we do this it should have a comment justifying
> it.
Agree on that one, decided on not implementing such a thing until it becomes
necessary.
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
Re: [PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.
On Tue, Sep 11, 2018 at 3:43 PM Ilya Biryukov via Phabricator < revi...@reviews.llvm.org> wrote: > ilya-biryukov added a comment. > > In https://reviews.llvm.org/D51747#1229066, @sammccall wrote: > > > A few thoughts here: > > > > - does CDB describe user or project preferences? unclear. > > > Agree, it's a mix, defaults are from the project but users can add extra > flags. > > > - "show this warning for code I build" is a higher bar than "show this > warning for code I edit". So CDB probably enables too few warnings. > > - Some warnings play well with -Werror (like uninit warnings), some > don't (like deprecated). -Werror projects often disable interesting > warnings. > > Agreed, editors are different from build. > > > I'm not sure we should strictly follow the CDB, but the bar to override > it should probably be high. > > WDYT in the long term about a more general mechanism (to allow users > override compiler or warning flags at the clangd level? > So that even if clangd is opinionated about the default warnings it > enables, users have an option to override according to their preferences. > Yeah, I can see making "extra clang flags" a clangd flag (at some point we really need .clangd config file or something...) The scary thing about the extra flags is how they interact with driver mode (clang-cl vs clang), but maybe that's the user's problem. > In https://reviews.llvm.org/D51747#1230420, @kadircet wrote: > > > if user wants to see all diagnostics as a list suddenly they will get > deprecations in that list as well :(. > > > Yeah, some level of noise is probably inevitable. > > > 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] Implement deprecation diagnostics with lower severity.
ilya-biryukov added a comment. In https://reviews.llvm.org/D51747#1229066, @sammccall wrote: > A few thoughts here: > > - does CDB describe user or project preferences? unclear. Agree, it's a mix, defaults are from the project but users can add extra flags. > - "show this warning for code I build" is a higher bar than "show this > warning for code I edit". So CDB probably enables too few warnings. > - Some warnings play well with -Werror (like uninit warnings), some don't > (like deprecated). -Werror projects often disable interesting warnings. Agreed, editors are different from build. > I'm not sure we should strictly follow the CDB, but the bar to override it > should probably be high. WDYT in the long term about a more general mechanism (to allow users override compiler or warning flags at the clangd level? So that even if clangd is opinionated about the default warnings it enables, users have an option to override according to their preferences. In https://reviews.llvm.org/D51747#1230420, @kadircet wrote: > if user wants to see all diagnostics as a list suddenly they will get > deprecations in that list as well :(. Yeah, some level of noise is probably inevitable. 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] Implement deprecation diagnostics with lower severity.
sammccall added a comment. In https://reviews.llvm.org/D51747#1230420, @kadircet wrote: > In https://reviews.llvm.org/D51747#1229066, @sammccall wrote: > > > In https://reviews.llvm.org/D51747#1228919, @ilya-biryukov wrote: > > > > > > Most of the value of adding an option is: if someone complains, we can > > > > tell them to go away :-) One possible corollary is: we shouldn't add > > > > the option until someone complains. I'm not 100% sure about that, > > > > though - not totally opposed to an option here. > > > > > > Any thoughts on tampering with provided compile args in the first place? > > > Is it fine for clangd to be opinionated about the warnings we choose to > > > always show to the users? > > > E.g. I'm a big fan of various uninitialized warnings, but nevertheless > > > don't think clangd should force them on everyone. > > > > > > A few thoughts here: > > > > - does CDB describe user or project preferences? unclear. > > - "show this warning for code I build" is a higher bar than "show this > > warning for code I edit". So CDB probably enables too few warnings. > > - Some warnings play well with -Werror (like uninit warnings), some don't > > (like deprecated). -Werror projects often disable interesting warnings. > > > > I'm not sure we should strictly follow the CDB, but the bar to override > > it should probably be high. Maybe the (default) behavior here should be > > "add -Wdeprecated -Wno-error=deprecated if -Werror is set"? > > > I agree with checking for -Werror and then providing -Wno-error as well, if > -Wdeprecated was not added already. I'm nervous enough about trying to ad-hoc parse the flags that I'd be tempted just to ad -Wno-error=deprecated without checking to see if -Werror is set - it's a no-op if not. But up to you. Ilya's suggestion of just respecting the compilation database also seems OK. It seems a bit sad for the users who won't see deprecation warnings because their project doesn't want to show them at build time. I'd lean towards adding this to see if anyone complains, but happy with whatever you two can agree on. > Then keeping it as warnings shouldn't be too much of a problem except the > case you mentioned, if user wants to see all diagnostics as a list suddenly > they will get deprecations in that list as well :(. I don't think that making them notes will avoid this though :-/ 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] Implement deprecation diagnostics with lower severity.
kadircet added a comment. In https://reviews.llvm.org/D51747#1229066, @sammccall wrote: > In https://reviews.llvm.org/D51747#1228919, @ilya-biryukov wrote: > > > > Most of the value of adding an option is: if someone complains, we can > > > tell them to go away :-) One possible corollary is: we shouldn't add the > > > option until someone complains. I'm not 100% sure about that, though - > > > not totally opposed to an option here. > > > > Any thoughts on tampering with provided compile args in the first place? Is > > it fine for clangd to be opinionated about the warnings we choose to always > > show to the users? > > E.g. I'm a big fan of various uninitialized warnings, but nevertheless > > don't think clangd should force them on everyone. > > > A few thoughts here: > > - does CDB describe user or project preferences? unclear. > - "show this warning for code I build" is a higher bar than "show this > warning for code I edit". So CDB probably enables too few warnings. > - Some warnings play well with -Werror (like uninit warnings), some don't > (like deprecated). -Werror projects often disable interesting warnings. > > I'm not sure we should strictly follow the CDB, but the bar to override it > should probably be high. Maybe the (default) behavior here should be "add > -Wdeprecated -Wno-error=deprecated if -Werror is set"? I agree with checking for -Werror and then providing -Wno-error as well, if -Wdeprecated was not added already. Then keeping it as warnings shouldn't be too much of a problem except the case you mentioned, if user wants to see all diagnostics as a list suddenly they will get deprecations in that list as well :(. 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] Implement deprecation diagnostics with lower severity.
sammccall added a comment. In https://reviews.llvm.org/D51747#1228919, @ilya-biryukov wrote: > > Most of the value of adding an option is: if someone complains, we can tell > > them to go away :-) One possible corollary is: we shouldn't add the option > > until someone complains. I'm not 100% sure about that, though - not totally > > opposed to an option here. > > Any thoughts on tampering with provided compile args in the first place? Is > it fine for clangd to be opinionated about the warnings we choose to always > show to the users? > E.g. I'm a big fan of various uninitialized warnings, but nevertheless don't > think clangd should force them on everyone. A few thoughts here: - does CDB describe user or project preferences? unclear. - "show this warning for code I build" is a higher bar than "show this warning for code I edit". So CDB probably enables too few warnings. - Some warnings play well with -Werror (like uninit warnings), some don't (like deprecated). -Werror projects often disable interesting warnings. I'm not sure we should strictly follow the CDB, but the bar to override it should probably be high. Maybe the (default) behavior here should be "add -Wdeprecated -Wno-error=deprecated if -Werror is set"? 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] Implement deprecation diagnostics with lower severity.
ilya-biryukov added a comment. > Most of the value of adding an option is: if someone complains, we can tell > them to go away :-) One possible corollary is: we shouldn't add the option > until someone complains. I'm not 100% sure about that, though - not totally > opposed to an option here. Any thoughts on tampering with provided compile args in the first place? Is it fine for clangd to be opinionated about the warnings we choose to always show to the users? E.g. I'm a big fan of various uninitialized warnings, but nevertheless don't think clangd should force them on everyone. 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] Implement deprecation diagnostics with lower severity.
sammccall added inline comments.
Comment at: clangd/ClangdServer.cpp:535
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
can you add a comment that these flags work with both gcc and clang-cl driver
modes?
It seems to be true. This is an important invariant (I should add a test for
it) that isn't obvious (there's no test today because we didn't know about
clang-cl at the time)
Comment at: clangd/ClangdServer.cpp:537
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated-declarations");
return std::move(*C);
as noted above I think we should also have -Wno-error=deprecated-declarations
(do you want all of -Wdeprecated, actually?)
Comment at: clangd/Diagnostics.cpp:299
+D.Severity =
+D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel;
return D;
not sure what the concrete benefits are from using Note rather than Warning.
It's semantically iffy, so if we do this it should have a comment justifying it.
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] Implement deprecation diagnostics with lower severity.
sammccall added a comment. In https://reviews.llvm.org/D51747#1227921, @ioeric wrote: > In https://reviews.llvm.org/D51747#1227089, @ilya-biryukov wrote: > > > > ! In https://reviews.llvm.org/D51747#1227719, @kadircet wrote: > > > > > >> ! In https://reviews.llvm.org/D51747#1227089, @ilya-biryukov wrote: > > > > > > Not sure if it's fine to hijack our own diagnostic-specific flags in to > > > clang command args. > > > > > > Cons that I see: > > > > > > 1. There is no way for the users to turn them off if they find them > > > non-useful. If we add a way, it would be more config parameters which > > > overlap with other mechanism that we have - compiler flags. > > > 2. Users who are used to having them as warnings will now see them as > > > notes. Again, no way to tweak this behavior. > > > > > > What's our use-case? Maybe we should ask the clients to add > > > -Wdeprecated if they care about those? > > > > > > PS In case I'm missing the context here, please let me know. > > > > Agree with you, I think it would be better to provide it as an option. > > https://reviews.llvm.org/D51724 with this one we added a way to show > > deprecated symbols on code completion responses and wanted to move forward > > with showing the ones that are already in existing code. > > > I also agree with you regarding options. A pattern I've observed (in some > infamous large codebase ;) is that warnings for deprecated symbols are > disabled in the compile command as they can introduce too much noise during > build, although it would make sense to show these warnings when user edits > the code (most of the time). I think there can be other diagnostics that are > more desirable as editor diagnostics than as compiler warnings/errors. So I'll be (somewhat) the naysayer regarding options. We still have to choose a default, and almost everyone will use it, including those who don't like it. So we still need the best possible default, and shouldn't let talk of options distract from this. Most of the value of adding an option is: if someone complains, we can tell them to go away :-) One possible corollary is: we shouldn't add the option until someone complains. I'm not 100% sure about that, though - not totally opposed to an option here. But if we add an option, we need to decide what the *other* option should be too, and this isn't obvious (should it be "suppress deprecation warnings" or "use compilation database as-is"? My vote for default behavior would be: add -Wdeprecated, and if deprecation diagnostics are **errors**, downgrade them to warnings. (Or better add `-Wdeprecated -Wno-error=deprecated`). But I'm biased by mostly using -Wno-deprecated -Werror codebases, if I didn't I'd probably prefer to just respect the compile-commands. (I'm not a fan of the downgrade-to-note behavior: notes do not carry the connotation of "bad", and deprecation warnings can have notes attached themselves which is confusing). 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] Implement deprecation diagnostics with lower severity.
ilya-biryukov added a comment. > I also agree with you regarding options. A pattern I've observed (in some > infamous large codebase ;) is that warnings for deprecated symbols are > disabled in the compile command as they can introduce too much noise during > build, although it would make sense to show these warnings when user edits > the code (most of the time). I think there can be other diagnostics that are > more desirable as editor diagnostics than as compiler warnings/errors. What kind of option are we talking about? (1) A flag to clangd or (2) a completely different way to expose deprecated methods (i.e. **not** diagnostics)? If (1), users already have a way to enable this by adding this flag when producing `compile_commands.json`. Not sure if adding a flag to clangd for tampering with one special-cased clang arg carries its weight, however produces `compile_commands.json` can set the flags they care about. (2) does seem useful and https://reviews.llvm.org/D51724 is a good example of it. As we have discussed recently, we could expose this as a form of semantic syntax highlighting (not available in clangd or LSP currently). 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] Implement deprecation diagnostics with lower severity.
ioeric added a comment. In https://reviews.llvm.org/D51747#1227089, @ilya-biryukov wrote: > Not sure if it's fine to hijack our own diagnostic-specific flags in to clang > command args. > > Cons that I see: > > 1. There is no way for the users to turn them off if they find them > non-useful. If we add a way, it would be more config parameters which overlap > with other mechanism that we have - compiler flags. > 2. Users who are used to having them as warnings will now see them as notes. > Again, no way to tweak this behavior. > > What's our use-case? Maybe we should ask the clients to add -Wdeprecated if > they care about those? > > PS In case I'm missing the context here, please let me know. Instead of In https://reviews.llvm.org/D51747#1227719, @kadircet wrote: > In https://reviews.llvm.org/D51747#1227089, @ilya-biryukov wrote: > > > Not sure if it's fine to hijack our own diagnostic-specific flags in to > > clang command args. > > > > Cons that I see: > > > > 1. There is no way for the users to turn them off if they find them > > non-useful. If we add a way, it would be more config parameters which > > overlap with other mechanism that we have - compiler flags. > > 2. Users who are used to having them as warnings will now see them as > > notes. Again, no way to tweak this behavior. > > > > What's our use-case? Maybe we should ask the clients to add -Wdeprecated > > if they care about those? > > > > PS In case I'm missing the context here, please let me know. > > > Agree with you, I think it would be better to provide it as an option. > https://reviews.llvm.org/D51724 with this one we added a way to show > deprecated symbols on code completion responses and wanted to move forward > with showing the ones that are already in existing code. I also agree with you regarding options. A pattern I've observed (in some infamous large codebase ;) is that warnings for deprecated symbols are disabled in the compile command as they can introduce too much noise during build, although it would make sense to show these warnings when user edits the code (most of the time). I think there can be other diagnostics that are more desirable as editor diagnostics than as compiler warnings/errors. 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] Implement deprecation diagnostics with lower severity.
kadircet added a comment. In https://reviews.llvm.org/D51747#1227089, @ilya-biryukov wrote: > Not sure if it's fine to hijack our own diagnostic-specific flags in to clang > command args. > > Cons that I see: > > 1. There is no way for the users to turn them off if they find them > non-useful. If we add a way, it would be more config parameters which overlap > with other mechanism that we have - compiler flags. > 2. Users who are used to having them as warnings will now see them as notes. > Again, no way to tweak this behavior. > > What's our use-case? Maybe we should ask the clients to add -Wdeprecated if > they care about those? > > PS In case I'm missing the context here, please let me know. Agree with you, I think it would be better to provide it as an option. https://reviews.llvm.org/D51724 with this one we added a way to show deprecated symbols on code completion responses and wanted to move forward with showing the ones that are already in existing code. Comment at: clangd/Diagnostics.cpp:299 +D.Severity = +D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel; return D; ioeric wrote: > kadircet wrote: > > Couldn't find a better way to check for this one. Category types are coming > > from tablegen file > > https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td#L128 > > which does not expose category id, at least I couldn't find even if it > > does. > Have you tried the Diagnostic ID (i.e. `Info.getID()`)? It matches the diag > kinds defined in `DiagnosticSemaKinds.inc` (e.g. `warn_deprecated`). Unfortunately these are also internal, https://github.com/llvm-mirror/clang/blob/master/lib/Basic/DiagnosticIDs.cpp#L97 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] Implement deprecation diagnostics with lower severity.
ilya-biryukov added a comment. Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args. Const that I see: 1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags. 2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior. What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those? PS In case I'm missing the context here, please let me know. 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] Implement deprecation diagnostics with lower severity.
ioeric added inline comments. Comment at: clangd/Diagnostics.cpp:299 +D.Severity = +D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel; return D; kadircet wrote: > Couldn't find a better way to check for this one. Category types are coming > from tablegen file > https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td#L128 > which does not expose category id, at least I couldn't find even if it does. Have you tried the Diagnostic ID (i.e. `Info.getID()`)? It matches the diag kinds defined in `DiagnosticSemaKinds.inc` (e.g. `warn_deprecated`). 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] Implement deprecation diagnostics with lower severity.
kadircet updated this revision to Diff 164268.
kadircet added a comment.
- Formatting.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
clangd/Diagnostics.cpp
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -38,6 +38,13 @@
return arg.Range == Range && arg.Message == Message;
}
+MATCHER_P3(Diag, Range, Message, Severity,
+ "Diag at " + llvm::to_string(Range) + " = [" + Message +
+ "] with Severity:" + llvm::to_string(Severity)) {
+ return arg.Range == Range && arg.Message == Message &&
+ arg.Severity == Severity;
+}
+
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -220,6 +227,46 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated",
+ DiagnosticsEngine::Note),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllOf(Diag(Test.range("x"), "'x' is deprecated",
+ DiagnosticsEngine::Note),
+WithNote(
+Diag(Test.range("xdeprecation"),
+ "'x' has been explicitly marked deprecated here");
+}
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+ Annotations Test(R"cpp(
+void bar();
+void foo() __attribute__((deprecated("", "bar")));
+int main() {
+ $deprecated[[foo]]();
+}
+ )cpp");
+ EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(WithFix(Fix(Test.range("deprecated"), "bar",
+ "change 'foo' to 'bar'";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -292,10 +292,11 @@
D.Message = Message.str();
D.InsideMainFile = InsideMainFile;
D.File = Info.getSourceManager().getFilename(Info.getLocation());
-D.Severity = DiagLevel;
D.Category = DiagnosticIDs::getCategoryNameFromID(
DiagnosticIDs::getCategoryNumberForDiag(Info.getID()))
.str();
+D.Severity =
+D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel;
return D;
};
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -534,6 +534,7 @@
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated-declarations");
return std::move(*C);
}
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -38,6 +38,13 @@
return arg.Range == Range && arg.Message == Message;
}
+MATCHER_P3(Diag, Range, Message, Severity,
+ "Diag at " + llvm::to_string(Range) + " = [" + Message +
+ "] with Severity:" + llvm::to_string(Severity)) {
+ return arg.Range == Range && arg.Message == Message &&
+ arg.Severity == Severity;
+}
+
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -220,6 +227,46 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated",
+ DiagnosticsEngine::Note),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllO
[PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.
kadircet added inline comments. Comment at: clangd/Diagnostics.cpp:299 +D.Severity = +D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel; return D; Couldn't find a better way to check for this one. Category types are coming from tablegen file https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td#L128 which does not expose category id, at least I couldn't find even if it does. 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] Implement deprecation diagnostics with lower severity.
kadircet created this revision.
kadircet added reviewers: ioeric, sammccall, ilya-biryukov, hokein.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay.
Adds deprecation warnings to diagnostics. Also lowers the severity from
warning to notes to not to annoy people that work on big codebases.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51747
Files:
clangd/ClangdServer.cpp
clangd/Diagnostics.cpp
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -38,6 +38,13 @@
return arg.Range == Range && arg.Message == Message;
}
+MATCHER_P3(Diag, Range, Message, Severity,
+ "Diag at " + llvm::to_string(Range) + " = [" + Message +
+ "] with Severity:" + llvm::to_string(Severity)) {
+ return arg.Range == Range && arg.Message == Message &&
+ arg.Severity == Severity;
+}
+
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -220,6 +227,48 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated",
+ DiagnosticsEngine::Note),
+WithNote(
+Diag(Test.range("foodeprecation"),
+ "'foo' has been explicitly marked deprecated here"))),
+ AllOf(Diag(Test.range("x"), "'x' is deprecated",
+ DiagnosticsEngine::Note),
+WithNote(
+Diag(Test.range("xdeprecation"),
+ "'x' has been explicitly marked deprecated here");
+}
+
+TEST(DiagnosticsTest, DiagnosticDeprecatedWithFix) {
+ Annotations Test(R"cpp(
+void bar();
+void foo() __attribute__((deprecated("", "bar")));
+int main() {
+ $deprecated[[foo]]();
+}
+ )cpp");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ WithFix(Fix(Test.range("deprecated"), "bar", "change 'foo' to
'bar'"))
+ ));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -292,10 +292,11 @@
D.Message = Message.str();
D.InsideMainFile = InsideMainFile;
D.File = Info.getSourceManager().getFilename(Info.getLocation());
-D.Severity = DiagLevel;
D.Category = DiagnosticIDs::getCategoryNameFromID(
DiagnosticIDs::getCategoryNumberForDiag(Info.getID()))
.str();
+D.Severity =
+D.Category == "Deprecations" ? DiagnosticsEngine::Note : DiagLevel;
return D;
};
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -534,6 +534,7 @@
// Inject the resource dir.
// FIXME: Don't overwrite it if it's already there.
C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+ C->CommandLine.push_back("-Wdeprecated-declarations");
return std::move(*C);
}
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -38,6 +38,13 @@
return arg.Range == Range && arg.Message == Message;
}
+MATCHER_P3(Diag, Range, Message, Severity,
+ "Diag at " + llvm::to_string(Range) + " = [" + Message +
+ "] with Severity:" + llvm::to_string(Severity)) {
+ return arg.Range == Range && arg.Message == Message &&
+ arg.Severity == Severity;
+}
+
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -220,6 +227,48 @@
}
}
+TEST(DiagnosticsTest, 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");
+ EXPECT_THAT(
+ TestTU::withCode(Test.code()).build().getDiagnostics(),
+ ElementsAre(
+ AllOf(Diag(Test.range("foo"), "'foo' is deprecated",
+
