Page MenuHomePhabricator

[clangd] Validate clang-tidy CheckOptions in clangd config
Needs ReviewPublic

Authored by njames93 on Jun 2 2022, 1:16 AM.

Details

Summary

Expand D92874 to also ensure all items in CheckOptions correspond to actual options in the clang-tidy checks.

Diff Detail

Event Timeline

njames93 created this revision.Jun 2 2022, 1:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 1:16 AM
njames93 requested review of this revision.Jun 2 2022, 1:16 AM
njames93 planned changes to this revision.Jun 10 2022, 7:33 AM

Gonna wait for the infrastructure from D127446 to land first, can reuse most of that implementation.

njames93 updated this revision to Diff 438388.Jun 20 2022, 7:34 AM

Reuse implementation from D127446

FWIW, I think validating the names makes sense but I don't think typo correction pays for itself here. @kadircet?

clang-tools-extra/clangd/TidyProvider.cpp
301

it seems strange that clang-tidy provides this API to query what checks are linked in, but it constructs an expensive object every time rather than just creating a static one once and returning a reference to it. (i.e. the memoization is on the caller side)
Should we fix that API instead?

I also agree with the typo correction verdict. In theory there'll be two cases:

  • typo correction helps, in which case it'll be obvious from the warning itself.
  • typo correction doesn't help, because the option doesn't exist at all, we'll be just showing a random option. I don't think how this'll be helpful.

So I don't think this is providing much of a value compared to the extra logic.

clang-tools-extra/clangd/TidyProvider.cpp
301

+1

I also agree with the typo correction verdict. In theory there'll be two cases:

  • typo correction helps, in which case it'll be obvious from the warning itself.
  • typo correction doesn't help, because the option doesn't exist at all, we'll be just showing a random option. I don't think how this'll be helpful.

So I don't think this is providing much of a value compared to the extra logic.

The correction only shows near misses, so it wouldn't show completely random options.
However it's only really there to prevent you having to look at the documentation for the check, so I'm happy to remove it.

clang-tools-extra/clangd/TidyProvider.cpp
301

The clang-tidy API is still a WIP. but it doesn't really make sense to memoize it on the tidy side as its s function that is only ever going to be called once in clang-tidy.

njames93 updated this revision to Diff 439004.Jun 22 2022, 7:06 AM

Remove typo correction support.