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

[Richard]

================
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.");
----------------
sbenza wrote:
> LegalizeAdulthood wrote:
> > sbenza wrote:
> > > LegalizeAdulthood wrote:
> > > > sbenza wrote:
> > > > > 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.
> > > > I think we are at that stage now; I do all early-out testing before constructing the replacement string.
> > > You are still generating a copy of the code before re-parsing it.
> > > Those will happen for anything that passes the matcher, even if they have no 'void' argument.
> > > removeVoidArgumentTokens should take the code as a StringRef directly from getText(), without converting to std::string first. GrammarLocation should also be a StringRef.
> > > Basically, avoid std::string until you are about to call diag().
> > I'll look into having getText return the StringRef and using that.  I don't understand the fuss over GrammarLocation, however.  Before I had it as a constant string that was just passed through and then another review wanted the duplication in the message eliminated, so I pulled that out.  Now you're saying that the string joining induced by the elimination of duplication is bad.  It doesn't seem possible to satisfy both comments.
> The problem with GrammarLocation is that it is declared as a 'const std::string &' and you are always passing literals.
> This is what StringRef is for. It will avoid the unnecessary temporary string.
> The signature would read like:
> 
>     void RemoveVoidArg::removeVoidArgumentTokens(
>         const MatchFinder::MatchResult &Result, SourceLocation StartLoc,
>         StringRef DeclText, StringRef GrammarLocation);
> 
> Then you can construct the Lexer with an std::string, like:
> 
>     clang::Lexer PrototypeLexer(StartLoc, Result.Context->getLangOpts(),
>                                 DeclText.data(), DeclText.data(),
>                                 DeclText.data() + DeclText.length());
> 
> 
Thanks for the explanation.  I'll try to take a look at this tonight.  There is similar code in some of the other reviews I have posted, so I'll fold that into those reviews as well.

http://reviews.llvm.org/D7639

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