================
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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
