This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Support clang-tidy configuration in clangd.
ClosedPublic

Authored by hokein on Dec 4 2018, 1:53 AM.

Details

Summary

This patch adds some basic supports for clang-tidy configurations in clangd:

  • clangd will respect .clang-tidy configurations for each file
  • we don't aim to support all clang-tidy options in clangd, only a small subset of condfigurations (options related to which checks will be enabled) are supported.
  • add a clang-tidy-checks CLI option that can override options from .clang-tidy file

Event Timeline

hokein created this revision.Dec 4 2018, 1:53 AM
hokein updated this revision to Diff 176591.Dec 4 2018, 3:24 AM

Minor cleanup

hokein edited the summary of this revision. (Show Details)Jan 11 2019, 3:04 AM
hokein added a reviewer: sammccall.

I think this patch is ready for review now. ClangTidy configurations are complicated, and we aim to support only a small subset of them (most are about controlling which checks are going to run in clangd). I'd like to get some initial feedbacks before making further changes.

Just a small comment wrt to a particular change in ClangdLSPServer. I haven't looked at the patch more closely, though.

clangd/ClangdLSPServer.h
131

Could we instead call getRealFS() when we try to initialize a clang-tidy options provider in main() and avoid changing this?
To avoid adding extra non-real-fs "modes of operation" to ClangdLSPServer. Unless you see other uses for this.

sammccall accepted this revision.Jan 15 2019, 4:25 AM

Nice! I think we should get to .clang-tidy files subsequently, but this is a great start and allows us to hook up the right set of checks in other environments.

clangd/ClangdLSPServer.h
131

We already have out-of-tree modifications to ClangdLSPServer to use non-real FSes.
Given that, I think this change is OK... though better still might be to move it into ClangdServer::Options

clangd/ClangdUnit.cpp
135

I have a hard time understanding this comment.
No need to replay events if nobody is listening?

272

this is a copy

538

this is a copy

clangd/ClangdUnit.h
82

Passing by value is OK here if deliberate, but let's try to avoid too many random copies below.

clangd/tool/ClangdMain.cpp
204

Maybe add a TODO or FIXME to respect .clang-tidy files?

This revision is now accepted and ready to land.Jan 15 2019, 4:25 AM
hokein updated this revision to Diff 182768.Jan 21 2019, 3:24 AM
hokein marked 7 inline comments as done.

Address review comments.

hokein added inline comments.Jan 21 2019, 3:25 AM
clangd/ClangdLSPServer.h
131

Yes, this is the main reason I did this change.

clangd/ClangdUnit.h
82

changed to const &

clangd/tool/ClangdMain.cpp
204

didn't get the point of the comment -- in this patch, clangd will read configurations from .clang-tidy files (FileOptionsProvider provides this functionality). This command-line flag is used to overwrite the .clang-tidy configurations,.

sammccall accepted this revision.Jan 21 2019, 3:39 AM
sammccall added inline comments.
clangd/tool/ClangdMain.cpp
204

Ah, I missed that FileOptionsProvider was actually used. Can you update the desc to something like "List of clang-tidy checks to run (overrides .clang-tidy files)"?

435

please move this up to a section near the other feature configuration stuff. e.g. below where CCOpts is initialized.

The Transport/ClangdLSPServer initialization is more closely related.

438

This prevents -clang-tidy-checks= from disabling all checks.
use ClangTidyChecks.getNumOccurrences() instead?

unittests/clangd/TestTU.cpp
39

Make this list an optional attribute to TestTU instead of hard-coding?

hokein updated this revision to Diff 182778.Jan 21 2019, 5:09 AM
hokein marked 5 inline comments as done.

Address comments.

hokein added inline comments.Jan 21 2019, 5:10 AM
clangd/tool/ClangdMain.cpp
438

To disable all checks, the -clang-tidy-checks should be -*. clang-tidy merges all configurations.

Actually we don't need this if, removed.

ilya-biryukov added inline comments.Jan 21 2019, 5:35 AM
clangd/ClangdLSPServer.h
131

It still feels that ClangdLSPServcer is closely tied to real fs, so I don't see how that makes things simpler. I wouldn't call the presence of out-of-tree modifications a good reason to do this change, at least not without tests and comments explaining why this needs to be configurable.

WRT to ClangdServer::Options, I believe this goes back to the previous discussion we had about putting the non-data configuration parameters (Index+FSProvider+CompilationsDB). I'd say put CompilationsDB into Options as well if you plan to put FSProvider, they should really live together (i.e. the reason we have out-of-tree modifications is closely tied to our custom CDB, so it makes sense for both to stay together).

FWIW, I still think they're FSProvider+CDB+Index should be separate parameters, but that goes back to the earlier conversation we had with @sammccall when the Options were added in the first place.

Not a big deal, just wanted to convey the intention of my original comment.

hokein marked an inline comment as done.Jan 22 2019, 1:37 AM
hokein added inline comments.
clangd/ClangdLSPServer.h
131

It still feels that ClangdLSPServcer is closely tied to real fs, so I don't see how that makes things simpler. I wouldn't call the presence of out-of-tree modifications a good reason to do this change, at least not without tests and comments explaining why this needs to be configurable.

This would simplifier our internal modification (just 1-2 line changes) -- ClangTidyOptionProvider needs to know FileSystem, we need a way to retrieve it (I believe both clangdLSPServer and ClangTidyOptionProvider should share the same non-real FS internally). An optional way is to add a GetFS method in ClangdLSPServer.

Anyway, I think this shouldn't block this patch, we could revisit it afterwards.

This revision was automatically updated to reflect the committed changes.