Page MenuHomePhabricator

[clang-tidy] Align usage of ClangTool interface with new VFS injection
Needs ReviewPublic

Authored by whisperity on Mar 30 2018, 8:39 AM.

Details

Reviewers
ilya-biryukov
Summary

Aligns the interface landed in D41535 to the patch which makes VFS injection more user-friendly. The interface how ClangTidy and the error reporter gets the filesystem to use has also been made more explicit: now ErrorReporter gets the exact Clang Tool('s filesystem layout) that Clang-Tidy used to run checks.

Diff Detail

Event Timeline

whisperity created this revision.Mar 30 2018, 8:39 AM
whisperity planned changes to this revision.Apr 5 2018, 11:54 AM
whisperity updated this revision to Diff 141207.Apr 5 2018, 1:09 PM

Update to be in line with contents in dependency patch.

alexfh removed a reviewer: alexfh.Apr 6 2018, 9:13 AM
ilya-biryukov added inline comments.
clang-tidy/ClangTidy.cpp
91

Why do we need to change the signature of ErrorReporter constructor?
Broadening the accepted interface does not seem right. It only needs the vfs and the clients could get vfs from ClangTool themselves.

clang-tidy/ClangTidy.h
229

I'm not sure if passing ClangTool here is an improvement.
Let's ask someone more familiar with clang-tidy. @hokein, @alexfh, WDYT?

clang-tidy/tool/CMakeLists.txt
19

Why do we need an extra dependency?

clang-tidy/tool/ClangTidyMain.cpp
432

Maybe use a shorter name, e.g. FileSystem?

whisperity added inline comments.Apr 9 2018, 5:50 AM
clang-tidy/ClangTidy.cpp
91

Is it okay interface-wise if the FS used by the ErrorReporter is not the same as the one used by the module that produced the errors? It seems like an undocumented consensus/convention that the same VFS should have been passed here.

clang-tidy/tool/CMakeLists.txt
19

In the current state of the patch, clang-tidy/tool/ClangTidyMain.cpp constructs the ClangTool which constructor requires a std::shared_ptr<PCHContainerOperations>. PCHContainerOperations's definition and implementation is part of the clangFrontend library, and without this dependency, there would be a symbol resolution error.

ilya-biryukov added inline comments.Apr 19 2018, 4:54 AM
clang-tidy/ClangTidy.cpp
91

I find actually documenting that the vfs should be the same to be a better option. Or even sharing the FileManager, but that might be more involved.

The problem of passing ClangTool is that it's a much more powerful than what we need (e.g. it allows to rerun clang-tidy, which is certainly not something that ErrorReporter should do).

clang-tidy/tool/CMakeLists.txt
19

Thanks for clarification.
Does it mean that to use ClangTool one needs both a dependency on clangTooling and clangFrontend?
That's weird, given that clangTooling itself depends on clangFrontend and it would be nice if the buildsystem actually propagated those.

whisperity added inline comments.Apr 19 2018, 7:22 AM
clang-tidy/tool/CMakeLists.txt
19

I'm not sure if you need to have both as a dependency just to use ClangTool. If you want to construct an empty PCHContainerOperations to pass as an argument, though, it seems you do.

One can wonder where the default argument's (which is a shared pointer to a default constructed object) expression's evaluation is in the object code or if passing nullptr breaks something.

You need the dependency because it's your code who wants to run the constructor of the PCH class.

Akin to D45094, pinging this too. ๐Ÿ™‚