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

[Alexander Kornienko]

In http://reviews.llvm.org/D7639#173909, @LegalizeAdulthood wrote:

> In http://reviews.llvm.org/D7639#136297, @xazax.hun wrote:
>
> > In http://reviews.llvm.org/D7639#136296, @LegalizeAdulthood wrote:
> >
> > > In http://reviews.llvm.org/D7639#136280, @xazax.hun wrote:
> > >
> > > > In http://reviews.llvm.org/D7639#136129, @LegalizeAdulthood wrote:
> > > >
> > > > > 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.
> > > >
> > > >
> > > > As far as I know, the point is that you can not do that without creating a null terminated string, however you do not need to do that. What you can do insead: you can lex the whole file buffer starting from the beginning of a declaration and you can make sure that you finish lexing on the end of the declaration (e.g.: end lexing when you match the closing paren of the declaration). This way you might have a bit more complicated lexing logic but you avoid heap allocation. So the point is: you construct the lexer using the whole file buffer starting from the right position and not relying on the source range at all to finish lexing.
> > >
> > >
> > > Relex the entire file just to avoid a heap allocation?  That seems a bit excessive.  Do we have any measurements on real code bases that show this to be the better approach?  I don't want to optimize like this without real data to show that it is worthwhile.
> > >
> > > Most of the time this string is going to either be "()" or "(void)".  Small string optimization means that there isn't even a heap allocation.
> >
> >
> > Well, you won't relex anything that does not need to be relexed. You only initialize the lexer using the whole buffer, so you get a null terminated string. But the starting point of the lexing would be the starting source location of the declaration you want to lex. And you would only lex the tokens you need. I think the lexer is lazy, so initializing with a bigger buffer should not cause any performance regressions in case you do not lex more tokens that you need. Of course measurements would tell this for sure.
>
>
> I'd like to address this in a subsequent changeset, since ti seems to be a matter of performance and not a matter of correctness with the existing code and I'd like to move this check forward.


This looks like a very small and quick change, so it would be nice to address this concern in the very first version of the check.


================
Comment at: clang-tidy/readability/RedundantVoidArgCheck.cpp:244
@@ +243,3 @@
+    removeVoidArgumentTokens(
+        Result, shrinkRangeByOne(SourceRange(CStyleCast->getLParenLoc(),
+                                             CStyleCast->getRParenLoc())),
----------------
Character-based offsets can be brittle in case escaped newlines and alternative tokens are used. I'd prefer getting the type range directly when it's possible. Here and for other casts something like this should work: `CStyleCast->getTypeInfoAsWritten()->getTypeLoc().getSourceRange()`.

Also, do we need separate functions for `ExplicitCastExpr` and two of its children classes?

================
Comment at: test/clang-tidy/readability-redundant-void-arg.c:77
@@ +76,3 @@
+
+// intentionally not LLVM style to check preservation of whitespace
+typedef void (function_ptr2)
----------------
Special tests for whitespace handling seem to be superfluous when you just need to check that no changes are made. Also, the invariant "no warnings => no fixes" seems to be safe to rely on, so you can remove all CHECK-FIXES and add one "CHECK-MESSAGES-NOT: warning:" so that the script runs the FileCheck on the output.

http://reviews.llvm.org/D7639

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