================
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:
> > 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.
I tried this and it almost works :-). StringRef doesn't put a NUL at the end
and the lexer needs that:
```
clang-tidy: /llvm/tools/clang/lib/Lex/Lexer.cpp:63: void
clang::Lexer::InitLexer(const char *, const char *, const char *): Assertion
`BufEnd[0] == 0 && "We assume that the input buffer has a null character at the
end" " to simplify lexing!"' failed.
```
So it looks like I still need to create a std::string right before lexing.
http://reviews.llvm.org/D7639
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
