Page MenuHomePhabricator

[clangd] Drop FS usage in ClangTidyOpts
Needs ReviewPublic

Authored by kadircet on Jun 17 2020, 3:37 AM.

Details

Reviewers
sammccall
Summary

Depends on D81998

Diff Detail

Event Timeline

kadircet created this revision.Jun 17 2020, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 3:37 AM
kadircet updated this revision to Diff 271342.Jun 17 2020, 5:22 AM
  • Accept an inner opt provider instead to enable wrapping other types of providers.
sammccall added inline comments.Jun 17 2020, 8:00 AM
clang-tools-extra/clangd/ClangdServer.h
46

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<ClangTidyOptions(StringRef)>?
It's reasonable to use a ClangTidyOptionsProvider to create them, though I'm not sure it actually saves anything.

njames93 added inline comments.
clang-tools-extra/clangd/tool/ClangdMain.cpp
495–498

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

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.