[PATCH] D82002: [clangd] Drop FS usage in ClangTidyOpts

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I'm of the idea that rather than having `ClangdTidyOptionsProvider` inherit 
form `tidy::ClangTidyOptionsProvider`, just have it as its own class. We don't 
need the interface offered by clang tidy here. It would solve the `must be 
threadsafe` comment issue as well as reduce the need for some unnecessary code 
in there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82002



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

[PATCH] D82002: [clangd] Drop FS usage in ClangTidyOpts

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:495-498
+bool HasChecks = false;
+for (const auto  : Sources)
+  HasChecks |= Source.first.Checks.hasValue();
+if (!HasChecks)

`if (llvm::none_of(Sources, [](const auto ) { return 
Source.first.Checks.hasValue(); }))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82002



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

[PATCH] D82002: [clangd] Drop FS usage in ClangTidyOpts

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:46
-/// to allow reading tidy configs from the VFS used for parsing.
-using ClangTidyOptionsBuilder = std::function;

Hmm, I like the idea of avoiding a custom type and just reusing the standard 
one here, but:
- taking someone else's interface (which has existing implementations, some of 
the key ones being subtly non-threadsafe) and slapping "must be threadsafe" on 
it seems like a recipe for bugs
- ClangTidyOptionsProvider is a really horrible interface that exposes several 
things we don't need

anything wrong with just std::function?
It's reasonable to use a ClangTidyOptionsProvider to create them, though I'm 
not sure it actually saves anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82002



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

[PATCH] D82002: [clangd] Drop FS usage in ClangTidyOpts

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82002

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -15,6 +15,7 @@
 #include "index/Background.h"
 #include "index/Serialization.h"
 #include "refactor/Rename.h"
+#include "support/FSProvider.h"
 #include "support/Path.h"
 #include "support/Shutdown.h"
 #include "support/Trace.h"
@@ -472,6 +473,57 @@
 const char TestScheme::TestDir[] = "/clangd-test";
 #endif
 
+// A thread-safe options provider suitable for use by ClangdServer. It also
+// provides some default checks if user has specified none.
+class ClangdTidyOptionsProvider : public clang::tidy::ClangTidyOptionsProvider {
+public:
+  ClangdTidyOptionsProvider(
+  std::unique_ptr Inner)
+  : Inner(std::move(Inner)) {
+assert(this->Inner);
+  }
+
+  std::vector getRawOptions(llvm::StringRef FileName) override {
+std::vector Sources;
+{
+  std::lock_guard Lock(Mu);
+  Sources = Inner->getRawOptions(FileName);
+}
+// 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...)
+bool HasChecks = false;
+for (const auto  : Sources)
+  HasChecks |= Source.first.Checks.hasValue();
+if (!HasChecks)
+  Sources.push_back(DefaultClangdSource);
+return Sources;
+  }
+
+  const tidy::ClangTidyGlobalOptions () override {
+std::lock_guard Lock(Mu);
+return Inner->getGlobalOptions();
+  }
+
+private:
+  std::mutex Mu;
+  std::unique_ptr Inner;
+  const OptionsSource DefaultClangdSource = []() -> OptionsSource {
+tidy::ClangTidyOptions Opts;
+// 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, "-clangd-opts"};
+  }();
+};
 } // namespace
 } // namespace clangd
 } // namespace clang
@@ -705,50 +757,20 @@
 TransportLayer = createPathMappingTransport(std::move(TransportLayer),
 std::move(*Mappings));
   }
-  // Create an empty clang-tidy option.
-  std::mutex ClangTidyOptMu;
-  std::unique_ptr
-  ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/
+  std::unique_ptr ClangTidyOptProvider;
   if (EnableClangTidy) {
 auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
 EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
 tidy::ClangTidyOptions OverrideClangTidyOptions;
 if (!ClangTidyChecks.empty())
   OverrideClangTidyOptions.Checks = ClangTidyChecks;
-ClangTidyOptProvider = std::make_unique(
-tidy::ClangTidyGlobalOptions(),
-/* Default */ EmptyDefaults,
-/* Override */ OverrideClangTidyOptions,
-FSProvider.view(/*CWD=*/llvm::None));
-Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
-   llvm::StringRef File) {
-  // This function must be thread-safe and tidy option providers are not.
-  tidy::ClangTidyOptions Opts;
-  {
-std::lock_guard Lock(ClangTidyOptMu);
-// 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");
-  }
-  

[PATCH] D82002: [clangd] Drop FS usage in ClangTidyOpts

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Accept an inner opt provider instead to enable wrapping other types of 
providers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82002

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -15,6 +15,7 @@
 #include "index/Background.h"
 #include "index/Serialization.h"
 #include "refactor/Rename.h"
+#include "support/FSProvider.h"
 #include "support/Path.h"
 #include "support/Shutdown.h"
 #include "support/Trace.h"
@@ -472,6 +473,57 @@
 const char TestScheme::TestDir[] = "/clangd-test";
 #endif
 
+// A thread-safe options provider suitable for use by ClangdServer. It also
+// provides some default checks if user has specified none.
+class ClangdTidyOptionsProvider : public clang::tidy::ClangTidyOptionsProvider {
+public:
+  ClangdTidyOptionsProvider(
+  std::unique_ptr Inner)
+  : Inner(std::move(Inner)) {
+assert(this->Inner);
+  }
+
+  std::vector getRawOptions(llvm::StringRef FileName) override {
+std::vector Sources;
+{
+  std::lock_guard Lock(Mu);
+  Sources = Inner->getRawOptions(FileName);
+}
+// 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...)
+bool HasChecks = false;
+for (const auto  : Sources)
+  HasChecks |= Source.first.Checks.hasValue();
+if (!HasChecks)
+  Sources.push_back(DefaultClangdSource);
+return Sources;
+  }
+
+  const tidy::ClangTidyGlobalOptions () override {
+std::lock_guard Lock(Mu);
+return Inner->getGlobalOptions();
+  }
+
+private:
+  std::mutex Mu;
+  std::unique_ptr Inner;
+  const OptionsSource DefaultClangdSource = []() -> OptionsSource {
+tidy::ClangTidyOptions Opts;
+// 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, "-clangd-opts"};
+  }();
+};
 } // namespace
 } // namespace clangd
 } // namespace clang
@@ -705,49 +757,20 @@
 TransportLayer = createPathMappingTransport(std::move(TransportLayer),
 std::move(*Mappings));
   }
-  // Create an empty clang-tidy option.
-  std::mutex ClangTidyOptMu;
-  std::unique_ptr
-  ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/
+  std::unique_ptr ClangTidyOptProvider;
   if (EnableClangTidy) {
 auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
 EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
 tidy::ClangTidyOptions OverrideClangTidyOptions;
 if (!ClangTidyChecks.empty())
   OverrideClangTidyOptions.Checks = ClangTidyChecks;
-ClangTidyOptProvider = std::make_unique(
-tidy::ClangTidyGlobalOptions(),
-/* Default */ EmptyDefaults,
-/* Override */ OverrideClangTidyOptions, FSProvider.view(llvm::None));
-Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
-   llvm::StringRef File) {
-  // This function must be thread-safe and tidy option providers are not.
-  tidy::ClangTidyOptions Opts;
-  {
-std::lock_guard Lock(ClangTidyOptMu);
-// 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",
- 

[PATCH] D82002: [clangd] Drop FS usage in ClangTidyOpts

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Depends on D81998 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82002

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -15,6 +15,7 @@
 #include "index/Background.h"
 #include "index/Serialization.h"
 #include "refactor/Rename.h"
+#include "support/FSProvider.h"
 #include "support/Path.h"
 #include "support/Shutdown.h"
 #include "support/Trace.h"
@@ -472,6 +473,51 @@
 const char TestScheme::TestDir[] = "/clangd-test";
 #endif
 
+// A thread-safe options provider suitable for use by ClangdServer. It also
+// provides some default checks if user has specified none.
+class ClangdTidyOptionsProvider : public clang::tidy::FileOptionsProvider {
+public:
+  ClangdTidyOptionsProvider(const tidy::ClangTidyOptions ,
+const ThreadSafeFS )
+  : FileOptionsProvider(tidy::ClangTidyGlobalOptions(),
+tidy::ClangTidyOptions(), OverrideOptions,
+FSProvider.view(llvm::None)) {}
+
+  std::vector getRawOptions(llvm::StringRef FileName) override {
+std::vector Sources;
+{
+  std::lock_guard Lock(Mu);
+  Sources = tidy::FileOptionsProvider::getRawOptions(FileName);
+}
+// 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...)
+bool HasChecks = false;
+for (const auto  : Sources)
+  HasChecks |= Source.first.Checks.hasValue();
+if (!HasChecks)
+  Sources.push_back(DefaultClangdSource);
+return Sources;
+  }
+
+private:
+  std::mutex Mu;
+  const OptionsSource DefaultClangdSource = []() -> OptionsSource {
+tidy::ClangTidyOptions Opts;
+// 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, "-clangd-opts"};
+  }();
+};
 } // namespace
 } // namespace clangd
 } // namespace clang
@@ -705,49 +751,15 @@
 TransportLayer = createPathMappingTransport(std::move(TransportLayer),
 std::move(*Mappings));
   }
-  // Create an empty clang-tidy option.
-  std::mutex ClangTidyOptMu;
-  std::unique_ptr
-  ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/
+  std::unique_ptr ClangTidyOptProvider;
   if (EnableClangTidy) {
-auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
-EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
 tidy::ClangTidyOptions OverrideClangTidyOptions;
 if (!ClangTidyChecks.empty())
   OverrideClangTidyOptions.Checks = ClangTidyChecks;
-ClangTidyOptProvider = std::make_unique(
-tidy::ClangTidyGlobalOptions(),
-/* Default */ EmptyDefaults,
-/* Override */ OverrideClangTidyOptions, FSProvider.view(llvm::None));
-Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
-   llvm::StringRef File) {
-  // This function must be thread-safe and tidy option providers are not.
-  tidy::ClangTidyOptions Opts;
-  {
-std::lock_guard Lock(ClangTidyOptMu);
-// 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",
-