[PATCH] Add readability-remove-void-arg check to clang-tidy

[Samuel Benzaquen]

================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:65
@@ +64,3 @@
+    Finder->addMatcher(typedefDecl(isExpansionInMainFile()).bind(TypedefId), this);
+    auto FunctionOrMemberPointer = anyOf(hasType(pointerType()), hasType(memberPointerType()));
+    Finder->addMatcher(fieldDecl(FunctionOrMemberPointer, isExpansionInMainFile()).bind(FieldId), this);
----------------
LegalizeAdulthood wrote:
> sbenza wrote:
> > The callbacks are not cheap. They generate string copies and then rerun the tokenizer on them.
> > These matchers should be more restricted to what we are looking for to avoid running the callbacks on uninteresting nodes.
> So... I need to write an expression that matches not just a pointer to function but a pointer to function with zero arguments?  I'll play with clang-query and see what I can figure out.
> 
> I managed to get some narrowing for everything but typedefDecl; I couldn't find any narrowing matchers for it.
It doesn't have to be a perfect matcher, but it should try to reject the most common false positives early.
Just by saying that it is a functionType() you get most of the varDecls out of the way without going into the callback.
This should be good for now.

================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:174
@@ +173,3 @@
+void RemoveVoidArg::processFieldDecl(const MatchFinder::MatchResult &Result, FieldDecl const *const Member) {
+    std::string const Text = getText(Result, *Member);
+    removeVoidArgumentTokens(Result, Member->getLocStart(), Text, "Redundant void argument list in field declaration.");
----------------
LegalizeAdulthood wrote:
> sbenza wrote:
> > Any reason all of these convert the StringRef into a std::string?
> > Couldn't they work with the StringRef directly?
> I tried switching it to a StringRef, but when I tried to combine it with other text to build a replacement for diag(), it got turned into Twine and then failed to match any signature for diag().  I'm open to suggestions for what you'd like to see instead.
Twine::str() constructs the string.
I don't mind constructing a string when a match happens. Those are rare.
But we should avoid allocating memory when no match happens.
Right now temporary memory allocations are a considerable part of clang-tidy's runtime. We should try to minimize that.

================
Comment at: clang-tidy/readability/RemoveVoidArg.h:38
@@ +37,3 @@
+private:
+    void processFunctionDecl(const ast_matchers::MatchFinder::MatchResult &Result, FunctionDecl const *const Function);
+
----------------
LegalizeAdulthood wrote:
> sbenza wrote:
> > alexfh wrote:
> > > Here and below: I'm against top-level `const` in function parameter types. It's just an implementation detail whether the method can modify its parameter. It doesn't make sense to pollute the interface with this.
> > > 
> > > Also, it's more common in Clang/LLVM code to put the const related to the pointed/referenced object in front:
> > > 
> > >   void processFunctionDecl(const ast_matchers::MatchFinder::MatchResult &Result, const FunctionDecl *Function);
> > Any reason why these are members?
> > Making them free functions in the .cpp file would avoid filling the header with these implementation details.
> > It would also remove the need to include Lexer.h here.
> They're members because ultimately on detection they ultimately call diag which is a member function.  Otherwise, I would have made them free functions.
They could still be free functions, that return some data instead of taking action directly.
But I understand now.

http://reviews.llvm.org/D7639

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/