sammccall added a comment.
Thanks for working on this!
I think the scope of this patch is probably bigger than we need, at least
initially:
- it adds a new TidyProvider system with a lot of flexibility. But our config
needs are quite static. The only flexibility we need initially is being able to
swap out reading .clang-tidy for a different strategy, and the ability to
replace the whole thing with a fixed config (for `-checks`)
- the clangd::Config based check configuration probably does need depend on
some refactoring of how ClangTidyOptions are created, but it doesn't need to
land in the same patch
- it adds caching around the file IO, which may be nice to have but is
complicated and unrelated and shouldn't be mixed with the refactoring
Do you want to take a shot at scoping this down? Is it useful if I sketch what
I mean for the refactoring part?
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:367
- // When set, provides clang-tidy options for a specific file.
- ClangTidyOptionsBuilder GetClangTidyOptions;
+ // OptionsPrivder for clang-tidy.
+ ClangdTidyProvider *ClangTidyProvider = nullptr;
----------------
new comment doesn't really say anything, revert to the old one?
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:250
if (Preamble && Preamble->StatCache)
- VFS = Preamble->StatCache->getConsumingFS(std::move(VFS));
+ VFS = Preamble->StatCache->getConsumingFS(VFS.get());
----------------
why this change?
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:310
CTContext->setCurrentFile(Filename);
+ dlog("ClangTidy configuration for file {0}: {1}", Filename,
+ tidy::configurationAsText(CTContext->getOptions()));
----------------
nit: move a few lines above to after the options-initialization if-stmt?
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:20
+/// The base class of all clang-tidy config providers for clangd. The option
+/// getters take a pointer to a Filesystem that should be used for any file IO.
+class ClangdTidyProvider {
----------------
Any reason to pass in a VFS to get a ClangTidyOptionsProvider here, rather than
having the subclass take a ThreadsafeFS in its constructor if it needs one?
This interface seems a bit strange, because not all implementations of
ClangTidyProvider would necessarily get their config from a VFS.
(Moreover, it seems like ClangTidyProvider could just be a
ClangTidyOptionsProvider with a threadsafety guarantee, if it weren't for the
fact that ClangTidyContext wants to take ownership of the optionsprovider,
which doesn't make a lot of sense - maybe we should fix that instead?)
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:21
+/// getters take a pointer to a Filesystem that should be used for any file IO.
+class ClangdTidyProvider {
+public:
----------------
we don't usually use a "Clangd" prefix in the clangd namespace.
(Clangd[LSP]Server being notable and very old exceptions)
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:40
+/// A Provider that will mimic the behaviour of tidy::FileOptionsProvider but
+/// it caches options retrieved in a thread safe manner. To be completely
thread
+/// safe it must be passed a thread safe filesystem when reading files.
----------------
how is the cache invalidated? (AFAICT currently it never is)
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:41
+/// it caches options retrieved in a thread safe manner. To be completely
thread
+/// safe it must be passed a thread safe filesystem when reading files.
+class FileTidyProvider : public ClangdTidyProvider {
----------------
We don't have a way to guarantee this, the vfs::FileSystems provided by
ThreadsafeFS are arbitrary (ThreadsafeFS is an extension point) and don't have
to be threadsafe.
If you need a filesystem that's threadsafe, that's what ThreadsafeFS is for.
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:74
+ // time, while preventing any thread updating the cache.
+ std::shared_timed_mutex CacheGuard;
+ llvm::StringMap<OptionsSource> CachedOptions;
----------------
reader-writer locks are a nice model but very commonly perform equal or worse
to plain mutexes in practice.
I think we should have evidence this is an improvement before using it.
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:85
+/// removing checks known to cause issues in clang-tidy.
+class CheckAdjustingTidyProvider : public FileTidyProvider {
+public:
----------------
Using a deep inheritance hierarchy here parallel to the hierarchy of
ClangTidyOptionsProvider is IMO too much complexity for the problem being
solved. Most of these classes want to be functions.
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:85
+/// removing checks known to cause issues in clang-tidy.
+class CheckAdjustingTidyProvider : public FileTidyProvider {
+public:
----------------
sammccall wrote:
> Using a deep inheritance hierarchy here parallel to the hierarchy of
> ClangTidyOptionsProvider is IMO too much complexity for the problem being
> solved. Most of these classes want to be functions.
in this design, we don't have a good way to use the check-adjustment together
with a non-file source of configs.
(At google we use this capability to support a different project configuration
format that's used instead of .clang-tidy files)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91029/new/
https://reviews.llvm.org/D91029
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
