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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 19 inline comments as done.
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 =
----------------
sammccall wrote:
> 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...
> 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).

That's what we do when working in llvm and clang, but convention around clangd is putting implementation details in anon namespaces. I would rather not stray from it here.


As for the general naming issues, i went with `UnusableTidyChecks`.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:219
+    //  - being reasonably efficient
+    Opts.ClangTidyOpts.Checks = llvm::join_items(
+        ",", "readability-misleading-indentation",
----------------
sammccall wrote:
> 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)
SG. Just as a note, we were already doing this at every request, but indirectly through `GetClangTidyOptions()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224