[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0464acd0197c: [clangd] Move clang-tidy check modifications 
into ClangdServer (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D83224?vs=285389=285407#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp

Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -26,9 +26,11 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -1194,6 +1196,44 @@
 }
 #endif
 
+TEST(ClangdServer, TidyOverrideTest) {
+  struct DiagsCheckingCallback : public ClangdServer::Callbacks {
+  public:
+void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
+std::vector Diagnostics) override {
+  std::lock_guard Lock(Mutex);
+  HadDiagsInLastCallback = !Diagnostics.empty();
+}
+
+std::mutex Mutex;
+bool HadDiagsInLastCallback = false;
+  } DiagConsumer;
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {"-xc++"};
+  auto Opts = ClangdServer::optsForTest();
+  Opts.GetClangTidyOptions = [](llvm::vfs::FileSystem &, llvm::StringRef) {
+auto Opts = tidy::ClangTidyOptions::getDefaults();
+// These checks don't work well in clangd, even if configured they shouldn't
+// run.
+Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+return Opts;
+  };
+  ClangdServer Server(CDB, FS, Opts, );
+  const char *SourceContents = R"cpp(
+struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); };
+namespace std { Foo&& move(Foo&); }
+void foo() {
+  Foo x;
+  Foo y = std::move(x);
+  Foo z = x;
+})cpp";
+  Server.addDocument(testPath("foo.h"), SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -808,23 +808,6 @@
 // FIXME: use the FS provided to the function.
 Opts = ClangTidyOptProvider->getOptions(File);
   }
-  if (!Opts.Checks) {
-// If the user hasn't configured clang-tidy checks at all, including
-// via .clang-tidy, give them a nice set of checks.
-// (This should be what the "default" options does, but it isn't...)
-//
-// These default checks are chosen for:
-//  - low false-positive rate
-//  - providing a lot of value
-//  - being reasonably efficient
-Opts.Checks = llvm::join_items(
-",", "readability-misleading-indentation",
-"readability-deleted-default", "bugprone-integer-division",
-"bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
-"bugprone-unused-raii", "bugprone-unused-return-value",
-"misc-unused-using-decls", "misc-unused-alias-decls",
-"misc-definitions-in-headers");
-  }
   return Opts;
 };
   }
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -40,6 +40,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
@@ -52,6 +53,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 namespace clang {
@@ -109,6 +111,41 @@
   ClangdServer::Callbacks *ServerCallbacks;
   bool TheiaSemanticHighlighting;
 };
+
+// Set of clang-tidy checks that don't work well in clangd, either due to
+// crashes or false positives.
+// Returns a clang-tidy filter string: -check1,-check2.
+llvm::StringRef getUnusableTidyChecks() {
+  static const std::string FalsePositives =
+  llvm::join_items(", ",
+   // Check relies on seeing ifndef/define/endif directives,
+   // clangd doesn't replay those when using 

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 285389.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp

Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -25,9 +25,11 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -1166,6 +1168,44 @@
 }
 #endif
 
+TEST(ClangdServer, TidyOverrideTest) {
+  struct DiagsCheckingCallback : public ClangdServer::Callbacks {
+  public:
+void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
+std::vector Diagnostics) override {
+  std::lock_guard Lock(Mutex);
+  HadDiagsInLastCallback = !Diagnostics.empty();
+}
+
+std::mutex Mutex;
+bool HadDiagsInLastCallback = false;
+  } DiagConsumer;
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {"-xc++"};
+  auto Opts = ClangdServer::optsForTest();
+  Opts.GetClangTidyOptions = [](llvm::vfs::FileSystem &, llvm::StringRef) {
+auto Opts = tidy::ClangTidyOptions::getDefaults();
+// These checks don't work well in clangd, even if configured they shouldn't
+// run.
+Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+return Opts;
+  };
+  ClangdServer Server(CDB, FS, Opts, );
+  const char *SourceContents = R"cpp(
+struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); };
+namespace std { Foo&& move(Foo&); }
+void foo() {
+  Foo x;
+  Foo y = std::move(x);
+  Foo z = x;
+})cpp";
+  Server.addDocument(testPath("foo.h"), SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -808,23 +808,6 @@
 // FIXME: use the FS provided to the function.
 Opts = ClangTidyOptProvider->getOptions(File);
   }
-  if (!Opts.Checks) {
-// If the user hasn't configured clang-tidy checks at all, including
-// via .clang-tidy, give them a nice set of checks.
-// (This should be what the "default" options does, but it isn't...)
-//
-// These default checks are chosen for:
-//  - low false-positive rate
-//  - providing a lot of value
-//  - being reasonably efficient
-Opts.Checks = llvm::join_items(
-",", "readability-misleading-indentation",
-"readability-deleted-default", "bugprone-integer-division",
-"bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
-"bugprone-unused-raii", "bugprone-unused-return-value",
-"misc-unused-using-decls", "misc-unused-alias-decls",
-"misc-definitions-in-headers");
-  }
   return Opts;
 };
   }
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -40,6 +40,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
@@ -52,6 +53,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 namespace clang {
@@ -109,6 +111,41 @@
   ClangdServer::Callbacks *ServerCallbacks;
   bool TheiaSemanticHighlighting;
 };
+
+// Set of clang-tidy checks that don't work well in clangd, either due to
+// crashes or false positives.
+// Returns a clang-tidy filter string: -check1,-check2.
+llvm::StringRef getUnusableTidyChecks() {
+  static const std::string FalsePositives =
+  llvm::join_items(", ",
+   // Check relies on seeing ifndef/define/endif directives,
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =
+  llvm::join_items(", ",
+   // Check can choke on invalid (intermediate) c++ code,
+   

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

sammccall wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > aaron.ballman wrote:
> > > > njames93 wrote:
> > > > > aaron.ballman wrote:
> > > > > > I suspect there are more checks that should be added here. For 
> > > > > > instance, much of `modernize-` is purely stylistic so it's easy to 
> > > > > > view as being false positives (like 
> > > > > > `modernize-use-trailing-return-types` or whatever it's called).
> > > > > I don't think that's what this list is for. This seems to be purely 
> > > > > for checks that don't run properly or crash inside clangd. 
> > > > > `modernize-use-trailing-return-types` is a very noisy check but 
> > > > > that's how it is when ran as normal clang-tidy. 
> > > > Ah, thank you for the explanation. Then how about 
> > > > `UnusableWithinClangd` or something other than `FalsePositives` for the 
> > > > name?
> > > The whole list is of checks that aren't usable in clangd, FalsePositives 
> > > is the specific reason. What's the problem with the name?
> > > What's the problem with the name?
> > 
> > If this is a list of checks that should be disabled because they have a ton 
> > of false positives, I think the name is fine and the list is incomplete. If 
> > this is a list of checks that should be disabled because they're not 
> > well-supported in clangd specifically, I think the name is misleading and 
> > the list is fine.
> > If this is a list of checks that should be disabled because they're not 
> > well-supported in clangd specifically, I think the name is misleading and 
> > the list is fine.
> 
> Fair enough - I think there's enough context to not need to repeat "in 
> clangd" here though. This code is in clangd, in a function with a comment 
> indicating that it's for checks that don't work well specifically in clangd. 
> (Maybe s/not suitable to be run through clangd/don't work well in clangd/ 
> though).
> Fair enough - I think there's enough context to not need to repeat "in 
> clangd" here though. This code is in clangd, in a function with a comment 
> indicating that it's for checks that don't work well specifically in clangd. 
> (Maybe s/not suitable to be run through clangd/don't work well in clangd/ 
> though).

That makes sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > njames93 wrote:
> > > > aaron.ballman wrote:
> > > > > I suspect there are more checks that should be added here. For 
> > > > > instance, much of `modernize-` is purely stylistic so it's easy to 
> > > > > view as being false positives (like 
> > > > > `modernize-use-trailing-return-types` or whatever it's called).
> > > > I don't think that's what this list is for. This seems to be purely for 
> > > > checks that don't run properly or crash inside clangd. 
> > > > `modernize-use-trailing-return-types` is a very noisy check but that's 
> > > > how it is when ran as normal clang-tidy. 
> > > Ah, thank you for the explanation. Then how about `UnusableWithinClangd` 
> > > or something other than `FalsePositives` for the name?
> > The whole list is of checks that aren't usable in clangd, FalsePositives is 
> > the specific reason. What's the problem with the name?
> > What's the problem with the name?
> 
> If this is a list of checks that should be disabled because they have a ton 
> of false positives, I think the name is fine and the list is incomplete. If 
> this is a list of checks that should be disabled because they're not 
> well-supported in clangd specifically, I think the name is misleading and the 
> list is fine.
> If this is a list of checks that should be disabled because they're not 
> well-supported in clangd specifically, I think the name is misleading and the 
> list is fine.

Fair enough - I think there's enough context to not need to repeat "in clangd" 
here though. This code is in clangd, in a function with a comment indicating 
that it's for checks that don't work well specifically in clangd. (Maybe s/not 
suitable to be run through clangd/don't work well in clangd/ though).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

sammccall wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > I suspect there are more checks that should be added here. For 
> > > > instance, much of `modernize-` is purely stylistic so it's easy to view 
> > > > as being false positives (like `modernize-use-trailing-return-types` or 
> > > > whatever it's called).
> > > I don't think that's what this list is for. This seems to be purely for 
> > > checks that don't run properly or crash inside clangd. 
> > > `modernize-use-trailing-return-types` is a very noisy check but that's 
> > > how it is when ran as normal clang-tidy. 
> > Ah, thank you for the explanation. Then how about `UnusableWithinClangd` or 
> > something other than `FalsePositives` for the name?
> The whole list is of checks that aren't usable in clangd, FalsePositives is 
> the specific reason. What's the problem with the name?
> What's the problem with the name?

If this is a list of checks that should be disabled because they have a ton of 
false positives, I think the name is fine and the list is incomplete. If this 
is a list of checks that should be disabled because they're not well-supported 
in clangd specifically, I think the name is misleading and the list is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[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: clang-tools-extra/clangd/ClangdServer.cpp:113
+
+// Set of clang-tidy checks that are not suitable to be run through clangd,
+// either due to crashes or false positives.

nit: "returns a clang-tidy filter string: -check1,-check2"



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:115
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =

aaron.ballman wrote:
> kadircet wrote:
> > aaron.ballman wrote:
> > > njames93 wrote:
> > > > Return by StringRef?
> > > How about `getDisabledClangTidyChecks()` (or literally any other name 
> > > than blacklist)?
> > thanks for bringing this to my attention, i will try to be more conscious 
> > next time.
> > 
> > I would prefer allow/deny as `disabled` might also be offensive in some 
> > contexts. Do you know if we already have some settlements around this one 
> > in the wider community?
> > thanks for bringing this to my attention, i will try to be more conscious 
> > next time.
> 
> No worries!
> 
> > I would prefer allow/deny as disabled might also be offensive in some 
> > contexts. Do you know if we already have some settlements around this one 
> > in the wider community?
> 
> I don't believe there's any consensus around avoiding use of "disabled" (we 
> use the term in a lot of places, especially when paired with "enabled"), but 
> I'd also be fine with allow/deny terminology instead.
> 
> As a minor drive-by comment, the function should also be marked `static` and 
> placed outside of the anonymous namespace (per our usual coding style).
I dislike "disabled" because it's vague: every check that's not enabled is 
disabled, but only a few of them are mentioned here.

I'd suggest BadlyBehavedTidyChecks or UnusableTidyChecks...



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > I suspect there are more checks that should be added here. For instance, 
> > > much of `modernize-` is purely stylistic so it's easy to view as being 
> > > false positives (like `modernize-use-trailing-return-types` or whatever 
> > > it's called).
> > I don't think that's what this list is for. This seems to be purely for 
> > checks that don't run properly or crash inside clangd. 
> > `modernize-use-trailing-return-types` is a very noisy check but that's how 
> > it is when ran as normal clang-tidy. 
> Ah, thank you for the explanation. Then how about `UnusableWithinClangd` or 
> something other than `FalsePositives` for the name?
The whole list is of checks that aren't usable in clangd, FalsePositives is the 
specific reason. What's the problem with the name?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:210
 GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (!Opts.ClangTidyOpts.Checks) {
+// If the user hasn't configured clang-tidy checks at all, including

kadircet wrote:
> njames93 wrote:
> > Should the `!` be removed the branches be swapped? Just looks cleaner imo, 
> > WDYT? 
> SGTM, will address once we've settled on the idea with Sam.
> 
> (and let this be a ping to him :D @sammccall )
I'd actually write this explicitly as hasValue() since the nullopt vs empty 
distinction is critical here



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:213
+// via .clang-tidy, give them a nice set of checks.
+// (This should be what the "default" options does, but it isn't...)
+//

this comment no longer belongs here, it's to do with the structure of the 
various ClangTidyOptionsProvider implementations, which aren't visible here. 
Fine to just drop it.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:219
+//  - being reasonably efficient
+Opts.ClangTidyOpts.Checks = llvm::join_items(
+",", "readability-misleading-indentation",

if you're going to do this on every request, might as well pull out the default 
set of checks into a function getDefaultTidyChecks() or so.

(So it's just joined once, but more importantly so we're consistent in how we 
separate the mechanism vs policy)



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:227
+  } else {
+// If user has enabled some checks, make sure clangd incompatible ones are
+// disabled.

nit: they haven't necessarily enabled checks (e.g. they could have specified 
`-checks=""` on the command line).

If the set of checks was configured?

(This is kind of a nit but did confuse 

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

njames93 wrote:
> aaron.ballman wrote:
> > I suspect there are more checks that should be added here. For instance, 
> > much of `modernize-` is purely stylistic so it's easy to view as being 
> > false positives (like `modernize-use-trailing-return-types` or whatever 
> > it's called).
> I don't think that's what this list is for. This seems to be purely for 
> checks that don't run properly or crash inside clangd. 
> `modernize-use-trailing-return-types` is a very noisy check but that's how it 
> is when ran as normal clang-tidy. 
Ah, thank you for the explanation. Then how about `UnusableWithinClangd` or 
something other than `FalsePositives` for the name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

aaron.ballman wrote:
> I suspect there are more checks that should be added here. For instance, much 
> of `modernize-` is purely stylistic so it's easy to view as being false 
> positives (like `modernize-use-trailing-return-types` or whatever it's 
> called).
I don't think that's what this list is for. This seems to be purely for checks 
that don't run properly or crash inside clangd. 
`modernize-use-trailing-return-types` is a very noisy check but that's how it 
is when ran as normal clang-tidy. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:115
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =

kadircet wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > Return by StringRef?
> > How about `getDisabledClangTidyChecks()` (or literally any other name than 
> > blacklist)?
> thanks for bringing this to my attention, i will try to be more conscious 
> next time.
> 
> I would prefer allow/deny as `disabled` might also be offensive in some 
> contexts. Do you know if we already have some settlements around this one in 
> the wider community?
> thanks for bringing this to my attention, i will try to be more conscious 
> next time.

No worries!

> I would prefer allow/deny as disabled might also be offensive in some 
> contexts. Do you know if we already have some settlements around this one in 
> the wider community?

I don't believe there's any consensus around avoiding use of "disabled" (we use 
the term in a lot of places, especially when paired with "enabled"), but I'd 
also be fine with allow/deny terminology instead.

As a minor drive-by comment, the function should also be marked `static` and 
placed outside of the anonymous namespace (per our usual coding style).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:115
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =

aaron.ballman wrote:
> njames93 wrote:
> > Return by StringRef?
> How about `getDisabledClangTidyChecks()` (or literally any other name than 
> blacklist)?
thanks for bringing this to my attention, i will try to be more conscious next 
time.

I would prefer allow/deny as `disabled` might also be offensive in some 
contexts. Do you know if we already have some settlements around this one in 
the wider community?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:115
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =

njames93 wrote:
> Return by StringRef?
How about `getDisabledClangTidyChecks()` (or literally any other name than 
blacklist)?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =

I suspect there are more checks that should be added here. For instance, much 
of `modernize-` is purely stylistic so it's easy to view as being false 
positives (like `modernize-use-trailing-return-types` or whatever it's called).



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:127
+   "-bugprone-use-after-move");
+  static const std::string BlackList =
+  llvm::join_items(", ", FalsePositives, CrashingChecks);

Similarly, rename this. I'd suggest `DisabledChecks`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:210
 GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (!Opts.ClangTidyOpts.Checks) {
+// If the user hasn't configured clang-tidy checks at all, including

njames93 wrote:
> Should the `!` be removed the branches be swapped? Just looks cleaner imo, 
> WDYT? 
SGTM, will address once we've settled on the idea with Sam.

(and let this be a ping to him :D @sammccall )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:210
 GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (!Opts.ClangTidyOpts.Checks) {
+// If the user hasn't configured clang-tidy checks at all, including

Should the `!` be removed the branches be swapped? Just looks cleaner imo, 
WDYT? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D83224#2133457 , @njames93 wrote:

> Do you think there should be a hidden command line option to disable 
> disabling blacklisted checks, purely to prevent hindering attempts to fix 
> these problematic checks.

I would expect those to be fixed and verified with unittests that are using 
TestTU, so the changes in ClangdServer shouldn't really be relevant to them. 
And if developer is trying to test something locally, i think it is better for 
them to just update the blacklist file(they are going to end up doing that 
anyways and perform possibly multiple compiles), so a flag to change blacklists 
behaviour might not be that meaningful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Do you think there should be a hidden command line option to disable disabling 
blacklisted checks, purely to prevent hindering attempts to fix these 
problematic checks.




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:115
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =

Return by StringRef?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224



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

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 275709.
kadircet added a comment.

- Add tests and also disable bugprone-use-after-move


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp

Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -23,9 +23,11 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -1121,6 +1123,42 @@
 }
 #endif
 
+TEST(ClangdServer, TidyOverrideTest) {
+  struct DiagsCheckingCallback : public ClangdServer::Callbacks {
+  public:
+void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
+std::vector Diagnostics) override {
+  std::lock_guard Lock(Mutex);
+  HadDiagsInLastCallback = !Diagnostics.empty();
+}
+
+std::mutex Mutex;
+bool HadDiagsInLastCallback = false;
+  } DiagConsumer;
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {"-xc++"};
+  auto Opts = ClangdServer::optsForTest();
+  Opts.GetClangTidyOptions = [](llvm::vfs::FileSystem &, llvm::StringRef) {
+auto Opts = tidy::ClangTidyOptions::getDefaults();
+Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+return Opts;
+  };
+  ClangdServer Server(CDB, FS, Opts, );
+  const char *SourceContents = R"cpp(
+struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); };
+namespace std { Foo&& move(Foo&); }
+void foo() {
+  Foo x;
+  Foo y = std::move(x);
+  Foo z = x;
+})cpp";
+  Server.addDocument(testPath("foo.h"), SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -729,23 +729,6 @@
 // FIXME: use the FS provided to the function.
 Opts = ClangTidyOptProvider->getOptions(File);
   }
-  if (!Opts.Checks) {
-// If the user hasn't configured clang-tidy checks at all, including
-// via .clang-tidy, give them a nice set of checks.
-// (This should be what the "default" options does, but it isn't...)
-//
-// These default checks are chosen for:
-//  - low false-positive rate
-//  - providing a lot of value
-//  - being reasonably efficient
-Opts.Checks = llvm::join_items(
-",", "readability-misleading-indentation",
-"readability-deleted-default", "bugprone-integer-division",
-"bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
-"bugprone-unused-raii", "bugprone-unused-return-value",
-"misc-unused-using-decls", "misc-unused-alias-decls",
-"misc-definitions-in-headers");
-  }
   return Opts;
 };
   }
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -39,6 +39,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
@@ -50,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 namespace clang {
@@ -107,6 +109,25 @@
   ClangdServer::Callbacks *ServerCallbacks;
   bool TheiaSemanticHighlighting;
 };
+
+// Set of clang-tidy checks that are not suitable to be run through clangd,
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =
+  llvm::join_items(", ",
+   // Check relies on seeing ifndef/define/endif directives,
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  static const std::string CrashingChecks =
+  llvm::join_items(", ",
+   // Check can choke on invalid (intermediate) c++ code,
+   // which is often the case when clangd tries to build an
+   // AST.
+   

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, aaron.ballman, usaxena95, arphaman, 
jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

This enables sharing the logic between standalone clangd and embedders
of it. The new approach should be same performance-wise, as it is only called
once per addDocument call.

This patch also introduces a blacklisting code path for disabling crashy or
high-noise tests, until we figure out a way to make them work with clangd-setup.

The biggest difference is the way we make use of preambles, hence those checks
can't see directives coming from the preamble section of the file. The second
thing is the fact that code might-not be compiling while clangd is trying to
build an AST, hence some checks might choke on those incomplete ASTs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83224

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -729,23 +729,6 @@
 // FIXME: use the FS provided to the function.
 Opts = ClangTidyOptProvider->getOptions(File);
   }
-  if (!Opts.Checks) {
-// If the user hasn't configured clang-tidy checks at all, including
-// via .clang-tidy, give them a nice set of checks.
-// (This should be what the "default" options does, but it isn't...)
-//
-// These default checks are chosen for:
-//  - low false-positive rate
-//  - providing a lot of value
-//  - being reasonably efficient
-Opts.Checks = llvm::join_items(
-",", "readability-misleading-indentation",
-"readability-deleted-default", "bugprone-integer-division",
-"bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
-"bugprone-unused-raii", "bugprone-unused-return-value",
-"misc-unused-using-decls", "misc-unused-alias-decls",
-"misc-definitions-in-headers");
-  }
   return Opts;
 };
   }
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -107,6 +107,17 @@
   ClangdServer::Callbacks *ServerCallbacks;
   bool TheiaSemanticHighlighting;
 };
+
+// Set of clang-tidy checks that are not suitable to be run through clangd,
+// either due to crashes or false positives.
+std::string getClangTidyBlacklist() {
+  std::string FalsePositives =
+  llvm::join_items(", ",
+   // Check relies on seeing ifndef/define/endif 
directives,
+   // clangd doesn't replay those when using a preamble.
+   "-llvm-header-guard");
+  return llvm::join_items(", ", FalsePositives);
+}
 } // namespace
 
 ClangdServer::Options ClangdServer::optsForTest() {
@@ -186,6 +197,28 @@
   if (GetClangTidyOptions)
 Opts.ClangTidyOpts =
 GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (!Opts.ClangTidyOpts.Checks) {
+// If the user hasn't configured clang-tidy checks at all, including
+// via .clang-tidy, give them a nice set of checks.
+// (This should be what the "default" options does, but it isn't...)
+//
+// These default checks are chosen for:
+//  - low false-positive rate
+//  - providing a lot of value
+//  - being reasonably efficient
+Opts.ClangTidyOpts.Checks = llvm::join_items(
+",", "readability-misleading-indentation",
+"readability-deleted-default", "bugprone-integer-division",
+"bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
+"bugprone-unused-raii", "bugprone-unused-return-value",
+"misc-unused-using-decls", "misc-unused-alias-decls",
+"misc-definitions-in-headers");
+  } else {
+// If user has enabled some checks, make sure clangd incompatible ones are
+// disabled.
+Opts.ClangTidyOpts.Checks = llvm::join_items(
+", ", *Opts.ClangTidyOpts.Checks, getClangTidyBlacklist());
+  }
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -729,23 +729,6 @@
 // FIXME: use the FS provided to the function.
 Opts = ClangTidyOptProvider->getOptions(File);
   }
-  if (!Opts.Checks) {
-// If the user hasn't configured clang-tidy checks at all, including
-// via .clang-tidy, give them a