[PATCH] Initial clang-tidy architecture
Daniel Jasper
djasper at google.com
Mon Jul 8 06:54:06 PDT 2013
================
Comment at: clang-tidy/ClangTidy.h:29
@@ +28,3 @@
+///
+/// This is used as an intermediate format to transport Diagnostics without a
+/// dependency on a SourceManager. The goal is to make Diagnostics flexible
----------------
Chandler Carruth wrote:
> Manuel Klimek wrote:
> > FIXME
> Yea, please make this a FIXME.
Done
================
Comment at: clang-tidy/ClangTidy.h:46
@@ +45,3 @@
+
+typedef SmallVector<ClangTidyError, 16> ClangTidyErrors;
+
----------------
Chandler Carruth wrote:
> This is still here...
Why wouldn't it? You have requested removing several other typedefs, but not this one. And this one seems to be quite useful (at least it is used in several places).
================
Comment at: clang-tidy/ClangTidy.h:61-64
@@ +60,6 @@
+
+ /// \brief Store a \c ClangTidyError.
+ ///
+ /// This should not be used directly.
+ void storeError(const ClangTidyError &Error) { Errors.push_back(Error); }
+
----------------
Chandler Carruth wrote:
> Why is this public?
It is called from ClangTidyDiagnosticConsumer. I have made this private and added a friend declaration.
================
Comment at: clang-tidy/ClangTidy.h:103-107
@@ +102,7 @@
+
+// FIXME: Implement confidence levels for displaying/fixing errors.
+//
+/// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
+/// Errors containing fixes are automatically applied.
+void handleErrors(ClangTidyErrors &Errors, bool Fix);
+
----------------
Chandler Carruth wrote:
> This interface doesn't make much since to me at this stage, but that's probably OK. I'm just commetning that I think there is more that needs to be understood here than just the FIXME you have.
Added a second FIXME.
================
Comment at: clang-tidy/ClangTidy.h:82
@@ +81,3 @@
+
+ void SetContext(ClangTidyContext *Ctx) { Context = Ctx; }
+ virtual void registerPPCallbacks(CompilerInstance &Compiler) {}
----------------
Chandler Carruth wrote:
> setContext
Done.
================
Comment at: clang-tidy/ClangTidy.cpp:84
@@ +83,3 @@
+ SmallVectorImpl<NamedCheck> &Checks;
+ ClangTidyContext &Context;
+};
----------------
Chandler Carruth wrote:
> This member is dead...
?
l. 75
================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:26
@@ +25,3 @@
+
+class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
+public:
----------------
Chandler Carruth wrote:
> As I can't imagine any really useful way to re-use this, I wonder about making it a file-local class inside ClangTidy.cpp?
And so I did at first, but then I found it really useful for the unit tests. Now, if we completely move away from unit-tests, that might change. Added FIXME.
================
Comment at: clang-tidy/ClangTidyModule.h:21-26
@@ +20,8 @@
+
+struct NamedCheck {
+ NamedCheck(StringRef Name, ClangTidyCheck *Check)
+ : Name(Name), Check(Check) {}
+ std::string Name;
+ ClangTidyCheck *Check;
+};
+
----------------
Chandler Carruth wrote:
> Wait, no documentation at all? ;] But this is like *the* API that you expect folks to be extending all the time.
>
> So, what is this? How does one use it? Who owns the Check?
This changed as per you comments below.
================
Comment at: unittests/clang-tidy/ClangTidyTest.cpp:16-17
@@ +15,4 @@
+
+TEST(ClangTidyTest, Basic) {
+}
+
----------------
Chandler Carruth wrote:
> Without some context, this doesn't make much sense. ;]
Removed this file for now. I think everything I wanted to test here is now actually tested by the FileCheck integration tests.
================
Comment at: clang-tidy/ClangTidy.h:48-49
@@ +47,4 @@
+
+/// \brief Every \c ClangTidyCheck reports errors through a \c DiagnosticEngine
+/// provided by this context.
+class ClangTidyContext {
----------------
Chandler Carruth wrote:
> Essentially every doxygen comment for an interface is this class feels really minimalist to me. Too much so. These interfaces are clearly intended to be used, potentially by clients that don't know what this is and are learning it by looking at these docs. I think we need to do more than just a brief summary. Usage examples and other reasonable documentation should be added to all of these reasonably public interfaces.
>
> Note that ClangTidyError is actually the best documented, despite potentially going away in the long term. ;]
I extended most doxygen comments.
================
Comment at: clang-tidy/ClangTidy.h:80
@@ +79,3 @@
+public:
+ virtual ~ClangTidyCheck() {}
+
----------------
Chandler Carruth wrote:
> Please provide anchors for these base classes with vtables.
Done.
================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:38-44
@@ +37,9 @@
+ const Diagnostic &Info) {
+ tooling::Replacements Replacements;
+ SourceManager &SourceMgr = Info.getSourceManager();
+ for (unsigned i = 0, e = Info.getNumFixItHints(); i != e; ++i) {
+ Replacements.insert(tooling::Replacement(
+ SourceMgr, Info.getFixItHint(i).RemoveRange.getBegin(), 0,
+ Info.getFixItHint(i).CodeToInsert));
+ }
+ SmallString<100> Buf;
----------------
Chandler Carruth wrote:
> Unifying the concept of Replacements and FixItHints seems like a great thing to pull out into its own FIXME and to form one of the obvious steps along the path of moving all of this into a generically more useful diagnostic library.
Done.
================
Comment at: clang-tidy/google/GoogleTidyModule.cpp:29
@@ +28,3 @@
+ExplicitConstructorCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+ Finder->addMatcher(constructorDecl().bind("construct"), this);
+}
----------------
Chandler Carruth wrote:
> Some stuff that I see how you probably want to do, but feel like would be really nice to include early on in the API documentation so that folks don't have to figure it out and to make sure everyone ends up on the same page:
>
> - What happens if I add multiple matchers? Why might i want to do that? Why might i not want to do that?
>
> - How can I get different callbacks to fire for different matchers? (The answer might be "you can't" rather than adding a way to do this, but then it should be justified.)
>
> - I have several different matched bits of the AST and diagnostics produced for them that are logically and importantly part of the same check. How do I implement that?
>
> - What is the lifetime of the Check? If I have a member that I use to keep track of state using AST node pointers, does that work? How does it break?
I added some basic comments to this effect (in ClangTidy.h), but I don't have clear answers to all of these points just yet.
================
Comment at: clang-tidy/google/GoogleTidyModule.cpp:48-49
@@ +47,4 @@
+ virtual void addChecks(SmallVectorImpl<NamedCheck> &Checks) {
+ Checks.push_back(NamedCheck("google-explicit-constructor",
+ new ExplicitConstructorCheck()));
+ }
----------------
Chandler Carruth wrote:
> Ahh, this is just an associative wrapper.
>
> Any particular reasons to not use an associative container? Or to hide the association from the type system by using thin wrappers around the container that expose methods with two arguments?
Made this an associative container hidden in ClangTidyCheckFactories.
I agree on per-TU lifetime as being a good/intuitive choice and changed the implementation accordingly.
I did not succeed in giving the CheckFactories value semantics. If you see a good way to do that, please let me know. However, I clearly defined the ownership in ClangTidyCheckFactories.
================
Comment at: unittests/clang-tidy/LLVMModuleTest.cpp:8
@@ +7,3 @@
+
+typedef ClangTidyTest<NamespaceCommentCheck> NamespaceCommentCheckTest;
+
----------------
Chandler Carruth wrote:
> No tests for the include order stuff yet? Guess that's fine, I'd rather see the better test infrastructure.
That is not properly implemented yet. Will be one of the next steps.
================
Comment at: unittests/clang-tidy/GoogleModuleTest.cpp:16-17
@@ +15,4 @@
+TEST_F(ExplicitConstructorCheckTest, Basic) {
+ EXPECT_EQ("class C { explicit C(int i); };",
+ runCheckOn("class C { C(int i); };"));
+}
----------------
Chandler Carruth wrote:
> Ugh, this testing machinery is ... pretty gross.
>
> We should really break down and pull GoogleMock into Clang and LLVM so we can use matchers instead of burying the logic. Specifically, it would be much nicer to just call the function to run the checks on the code, get the object back, and have the match select whether it tested by applying the fixes or by inspecting the diagnostic messages, etc.
>
> As it is, there is no really good way to test the diagnostic messages produced, only the fixes applied to the source code.
>
> I think eventually you'll need a better testing system than this. I think the ideal testing system would be something like what we use for clang -fixit where we first run the diagnostic verifier which uses one set of comments in the code to validate diagnostics produced, and then we run again and hand the result to FileCheck which uses the CHECK comments to make assertions about the transformed source.
>
> I think the command line tool should already work to write the second kind of test, and it would be equivalent to this but (imo) somewhat more in keeping with the general Clang testing processes. The first should be easy to add to the command line tool when the diagnostic engine is factored out of Clang's innards and made into a stand-alone useful library as part of that will also make the verifier available. Then it's just minor wiring to have a --verify flag.
Added FIXME in ClangTidyTest.h.
And also add the <clang-tools-extra>/test/clang-tidy directory to this patch. I think that contains some of the tests you are envisioning and I just forgot to include it before :-/.
http://llvm-reviews.chandlerc.com/D884
More information about the cfe-commits
mailing list