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 me...)
================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1145
+ auto Opts = tidy::ClangTidyOptions::getDefaults();
+ Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+ return Opts;
----------------
Maybe add a comment like "these checks don't work well in clangd, even if
configured they shouldn't run"
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
