[PATCH] Add readability-redundant-void-arg check to clang-tidy
Richard
legalize at xmission.com
Sat Mar 7 15:56:25 PST 2015
================
Comment at: clang-tidy/readability/RedundantVoidArg.cpp:41
@@ +40,3 @@
+bool protoTypeHasNoParms(QualType QT) {
+ if (const auto PT = QT->getAs<PointerType>()) {
+ QT = PT->getPointeeType();
----------------
alexfh wrote:
> Please use either "auto" or "const auto *" consistently. This function has usages of both.
Fixed
================
Comment at: clang-tidy/readability/RedundantVoidArg.cpp:148
@@ +147,3 @@
+ std::string DeclText = getText(Result, Start, BeforeBody);
+ removeVoidArgumentTokens(Result, Start, DeclText, "function definition");
+ } else {
----------------
xazax.hun wrote:
> You can also pass std::string to an llvm::StringRef. In the llvm codebase it is a rule of thumb to use StringRef instead of const std::string&.
Fixed.
================
Comment at: clang-tidy/readability/RedundantVoidArg.cpp:158
@@ +157,3 @@
+ const std::string &DeclText, StringRef GrammarLocation) {
+ clang::Lexer PrototypeLexer(StartLoc, Result.Context->getLangOpts(),
+ DeclText.data(), DeclText.data(),
----------------
xazax.hun wrote:
> It is possible to create a lexer without creating an std::string. You can check an example in: http://reviews.llvm.org/D7375 (look for getBuffer and getCharacterData). Of course, this way you have to make sure to end lexing after the interested position is left, but you avoid the heap allocation.
This suggestion has been made several times during this review and every time I try it I get this error at runtime:
```
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.
```
If you can show me how to lex a SourceRange without getting that error, I'm happy to change it. I tried the method shown in the linked diff and I get the same error.
However, one good thing came from the experiment. This checker only creates fixits that remove text, so I don't need to compute replacement text and this simplified the replacement code a little bit more.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:42
@@ +41,3 @@
+ if (const auto PT = QT->getAs<PointerType>()) {
+ if (const auto FP = PT->getPointeeType()->getAs<FunctionProtoType>()) {
+ return FP->getNumParams() == 0;
----------------
sbenza wrote:
> Remove the duplicate logic.
> Could be something like:
>
> ```
> if (const auto PT = QT->getAs<PointerType>()) {
> QT = PT->getPointeeType();
> }
> if (const auto *MPT = QT->getAs<MemberPointerType>()) {
> QT = MPT->getPointeeType();
> }
> if (const auto FP = QT->getAs<FunctionProtoType>()) {
> return FP->getNumParams() == 0;
> }
> ```
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:73
@@ +72,3 @@
+ const LangOptions &LangOpts = Compiler.getLangOpts();
+ CPlusPlusFile_ = LangOpts.CPlusPlus || LangOpts.CPlusPlus11 ||
+ LangOpts.CPlusPlus14 || LangOpts.CPlusPlus1z;
----------------
xazax.hun wrote:
> As far as I can remember when the code compiled in e.g.: CPlusPlus14 mode, the CPlusPlus11 and CPlusPlus flags are also turned on. It might be enough to check CPlusPlus.
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:176
@@ +175,3 @@
+ Token ProtoToken;
+ const auto Diagnostic = "redundant void argument list in " + GrammarLocation;
+ while (!PrototypeLexer.LexFromRawLexer(ProtoToken)) {
----------------
alexfh wrote:
> s/const auto/std::string/
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:186
@@ +185,3 @@
+ if (ProtoToken.is(tok::TokenKind::kw_void) ||
+ (ProtoToken.is(tok::TokenKind::raw_identifier) &&
+ ProtoToken.getRawIdentifier() == "void")) {
----------------
alexfh wrote:
> You don't need to check for both kw_void and raw_identifier + "void". You won't see the former unless you resolve the identifier and set the token kind yourself.
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:219
@@ +218,3 @@
+ if (protoTypeHasNoParms(Typedef->getUnderlyingType())) {
+ auto Text = getText(Result, *Typedef);
+ removeVoidArgumentTokens(Result, Typedef->getLocStart(), Text, "typedef");
----------------
alexfh wrote:
> Please use `StringRef` instead of `auto` here (see the other code review thread for an explanation).
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:227
@@ +226,3 @@
+ if (protoTypeHasNoParms(Member->getType())) {
+ const auto Text = getText(Result, *Member);
+ removeVoidArgumentTokens(Result, Member->getLocStart(), Text,
----------------
alexfh wrote:
> StringRef
Fixed
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:241
@@ +240,3 @@
+ .getLocWithOffset(-1);
+ const auto Text = getText(Result, Begin, InitStart);
+ removeVoidArgumentTokens(Result, Begin, Text,
----------------
alexfh wrote:
> s/const auto/StringRef/
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:255
@@ +254,3 @@
+ if (protoTypeHasNoParms(NamedCast->getTypeAsWritten())) {
+ const auto AngleBrackets = NamedCast->getAngleBrackets();
+ const auto Begin = AngleBrackets.getBegin().getLocWithOffset(1);
----------------
alexfh wrote:
> s/const auto/SourceLocation/
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:267
@@ +266,3 @@
+ if (protoTypeHasNoParms(CStyleCast->getTypeAsWritten())) {
+ const auto Begin = CStyleCast->getLParenLoc().getLocWithOffset(1);
+ const auto End = CStyleCast->getRParenLoc().getLocWithOffset(-1);
----------------
alexfh wrote:
> auto
Fixed
================
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);
----------------
sbenza wrote:
> 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.
I did find ways to reject more cases before beginning a lexical scan of text, so I'm opting out earlier than I was initially.
I couldn't find the right matcher incantation with `clang-query` to narrow it further in the matcher instead of in the callback.
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:78
@@ +77,3 @@
+ BoundNodes const &Nodes = Result.Nodes;
+ if (FunctionDecl const *const Function = Nodes.getNodeAs<FunctionDecl>(FunctionId)) {
+ processFunctionDecl(Result, Function);
----------------
sbenza wrote:
> why not auto in this one? You used it on all other cases below.
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:169
@@ +168,3 @@
+void RemoveVoidArg::processTypedefDecl(const MatchFinder::MatchResult &Result, TypedefDecl const *const Typedef) {
+ std::string const Text = getText(Result, *Typedef);
+ removeVoidArgumentTokens(Result, Typedef->getLocStart(), Text, "Redundant void argument list in typedef.");
----------------
xazax.hun wrote:
> I think in the LLVM codebase consts tend to be written before the type and not after.
Fixed
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:170
@@ +169,3 @@
+ std::string const Text = getText(Result, *Typedef);
+ removeVoidArgumentTokens(Result, Typedef->getLocStart(), Text, "Redundant void argument list in typedef.");
+}
----------------
alexfh wrote:
> 1. The "Redundant void argument list in" part is repeated everywhere. Maybe just pass the description of the location (here - "typedef") as an argument to `removeVoidArgumentTokens`?
>
> 2. Clang-tidy messages should use the same style as Clang diagnostics: start with a lower-case letter, and no trailing period.
Fixed
================
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:
> 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.
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:105
@@ +104,3 @@
+ const BoundNodes &Nodes = Result.Nodes;
+ if (const auto Function = Nodes.getNodeAs<FunctionDecl>(FunctionId)) {
+ processFunctionDecl(Result, Function);
----------------
alexfh wrote:
> I think, the "const" here makes the code less readable and doesn't add any value as the variable is only visible to a one-line statement. Same for the other conditions.
Fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:207
@@ +206,3 @@
+void RemoveVoidArg::processFieldDecl(const MatchFinder::MatchResult &Result,
+ const FieldDecl *const Member) {
+ const std::string Text = getText(Result, *Member);
----------------
alexfh wrote:
> Please remove top-level const in the method definitions as well.
Fixed
================
Comment at: clang-tidy/readability/RemoveVoidArg.h:26
@@ +25,3 @@
+/// int foo(void); ==> int foo();
+class RemoveVoidArg : public ClangTidyCheck
+{
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Formatting is off in many places. Could you clang-format the code so I don't need to point to every nit?
> I just assumed a final pass through clang-format before being committed; I haven't gone through CLion trying to make it follow clang's/llvm's indent rules.
Fixed.
================
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:
> > 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.
> They'd essentially have to return all the arguments in this statement:
>
> ```
> diag(VoidLoc, Diagnostic) << FixItHint::CreateRemoval(VoidRange);
> ```
> ...which feels tortured just to avoid creating member functions on the implementation.
const issues fixed.
================
Comment at: clang-tidy/readability/RemoveVoidArg.h:38
@@ +37,3 @@
+private:
+ bool CPlusPlusFile_;
+
----------------
alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > 1. You don't need to store this flag as it's easy and cheap to check it on each call to `check()`: it's available as `Result.Context->getLangOpts().CPlusPlus`.
> > > 2. This name violates LLVM Naming conventions: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> > Thanks for showing me where I could get that option. I drilled around in the code for a while and couldn't find it except hanging off the Compiler. What would be really nice is if I could get ahold of that flag when I'm asked to add matchers as I could simply not add any matchers in that case, but `registerPPCallbacks` is called after `registerMatchers`. Can I navigate to it from `ClangTidyContext`?
> Language options only become available when a compiler instance is being initialized. The matchers are registered way before that and then multiple different compilations may be run potentially with different options (if multiple translation units have been passed to clang-tidy).
>
> So there's no way to learn what language will be analyzed when we register matchers.
Fixed.
================
Comment at: test/clang-tidy/readability-remove-void-arg.cpp:28
@@ +27,3 @@
+int (*returns_fn_void_int(void))(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: Redundant void argument list in function declaration. [readability-remove-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: Redundant void argument list in function declaration. [readability-remove-void-arg]
----------------
alexfh wrote:
> Repeating the whole message each time makes the test harder to read. Please leave the whole message once and shorten all other occurrences, e.g.:
>
> // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} in function declaration
Fixed
================
Comment at: test/clang-tidy/readability-remove-void-arg.cpp:145
@@ +144,3 @@
+// CHECK-FIXES: {{^}}typedef void (function_ptr2){{$}}
+// CHECK-FIXES-NEXT: {{^ }}({{$}}
+// CHECK-FIXES-NEXT: {{^ $}}
----------------
alexfh wrote:
> It would be more readable with a single regular expression:
> {{^ \($}}
Fixed.
http://reviews.llvm.org/D7639
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the cfe-commits
mailing list