[Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about variable cast to void

[IdrissRio]

Summary

Hello, i would like to suggest a fix for one of the checks in clang-tidy.The bug was reported in https://bugs.llvm.org/show_bug.cgi?id=32575 where you can find more information.

For example:

template <typename T0>
struct S {
  template <typename T>
  void g() const {
    int a;
    (void)a;
  }
};

void f() {
  S<int>().g<int>();
}

this piece of code should not trigger any warning by the check modernize-redundant-void-arg but when we execute the following command

clang_tidy -checks=-*,modernize-redundant-void-arg test.cpp -- -std=c++11

we obtain the following warning:

/Users/eco419/Desktop/clang-tidy.project/void-redundand_2/test.cpp:6:6: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]

(void)a;
 ^~~~

[lebedev.ri]

This looks weird.
It will now completely skip all the explicitly instantiated functions, no?
I'd think the problem that needs to be fixed is that it considers the local variable int a_template; to be in the function argument list.

[IdrissRio]

In D52135#1235950, @lebedev.ri wrote:

It will now completely skip all the explicitly instantiated functions, no?

In my opinion, for this kind of check, we don't have the necessity to take in consideration the function instantiation. We only have to consider the function definition/declaration.

I'd think the problem that needs to be fixed is that it considers the local variable int a_template; to be in the function argument list.

No the problem is that the ast_matcher find 2 occurrence of g_template: the first is the definition of g_template() and the second is the instantiation of g_template. The second is the one that create the problem since it start to iterate all over the body of the function trying to fine the the tokens '( void )' that in this case is this guiltless void cast : (void)a_template.

[JonasToth]

Thank you for working on this!

Could you please add a test, to show that non-templated function are still diagnosed correctly?
The isInstantiated would only apply to templated functions.

Why is the cast to void diagnosed anyway? It is not an argument nor is it used in a function context. Do you have an explaination for that?

[IdrissRio]

Thank you @JonasToth.
I have update the patch and i have added two cases. The first an
instantiation of a non-templated function. The second is the instantiation
of a templatic funciton.

In D52135#1236220, @JonasToth wrote:

Why is the cast to void diagnosed anyway? It is not an argument nor is it used in a function context. Do you have an explaination for that?

Yes, this because when we find a function and the process it we are trying to get the beginning and the end of the function in terms of source location. In particular we have

 if (Function->isThisDeclarationADefinition()){
   ...
}else{
  removeVoidArgumentTokens(Result, Function->getSourceRange(),
                             "function declaration");
}

In this case Function is nor a declaration or definition so we fall in the else case and so we are going to take the entire sourceRange of Function.
The function RedundantVoidArgCheck::removeVoidArgumentTokens iterate over this sourceRange and if an occurrence of '(void)' is found this is replaced with '()'. In the our case removeVoidArgumentTokens will find this occurrence in (void)a.

[JonasToth]

In D52135#1236220, @JonasToth wrote:

Why is the cast to void diagnosed anyway? It is not an argument nor is it used in a function context. Do you have an explaination for that?

Yes, this because when we find a function and the process it we are trying to get the beginning and the end of the function in terms of source location. In particular we have

 if (Function->isThisDeclarationADefinition()){
   ...
}else{
  removeVoidArgumentTokens(Result, Function->getSourceRange(),
                             "function declaration");
}

In this case Function is nor a declaration or definition so we fall in the else case and so we are going to take the entire sourceRange of Function.
The function RedundantVoidArgCheck::removeVoidArgumentTokens iterate over this sourceRange and if an occurrence of '(void)' is found this is replaced with '()'. In the our case removeVoidArgumentTokens will find this occurrence in (void)a.

Thank you for the clarification!

test/clang-tidy/modernize-redundant-void-arg.cpp

510

Nit. The formatting at the brace is a bit off.

Could you please add a testcase for a templated function as well?

template <typename T>
void bla(void) {

}

I think every possibility is then covered.

[IdrissRio]

Thank you @JonasToth. As you suggest I have added the test for the
templatic function.

[JonasToth]

LG from my side.
I guess you found this in a real code base, if so could you please verify its fixed now.

If @aaron.ballman, @alexfh or @hokein have no complaints it can be committed. Do you have rights? Otherwise I can commit for you if you want.

[JonasToth]

test/clang-tidy/modernize-redundant-void-arg.cpp

495

Nit: please put that diagnostic on one line, even if its longer then the normal column limit.

[IdrissRio]

In D52135#1236690, @JonasToth wrote:

LG from my side.
I guess you found this in a real code base, if so could you please verify its fixed now.

Actually I'm searching for the bug in bugzilla and I try to fix them.

If @aaron.ballman, @alexfh or @hokein have no complaints it can be committed. Do you have rights? Otherwise I can commit for you if you want.

I have the commits right. If is not a problem I will commit by my self. Thank you.

[JonasToth]

Ok, if possible double check that it actually worked, otherwise not ;)

Perfect! You can ofcourse commit yourself

Am 17.09.2018 um 13:47 schrieb Idriss via Phabricator:

IdrissRio added a comment.

In https://reviews.llvm.org/D52135#1236690, @JonasToth wrote:

LG from my side.
I guess you found this in a real code base, if so could you please verify its fixed now.

Actually I'm searching for the bug in bugzilla and I try to fix them.

If @aaron.ballman, @alexfh or @hokein have no complaints it can be committed. Do you have rights? Otherwise I can commit for you if you want.

I have the commits right. If is not a problem I will commit by my self. Thank you.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D52135

[aaron.ballman]

LGTM with the nits corrected (there are still some formatting concerns).

test/clang-tidy/modernize-redundant-void-arg.cpp

541

Formatting here looks off as well -- you should run the patch through clang-format: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

clang-tidy/modernize/RedundantVoidArgCheck.cpp

 
 } // namespace
 
 void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()),
-                                  unless(isExternC()))
+                                  unless(isInstantiated()), unless(isExternC()))
                          .bind(FunctionId),
                      this);
   Finder->addMatcher(typedefNameDecl().bind(TypedefId), this);
   auto ParenFunctionType = parenType(innerType(functionType()));
   auto PointerToFunctionType = pointee(ParenFunctionType);
   auto FunctionOrMemberPointer =
       anyOf(hasType(pointerType(PointerToFunctionType)),
             hasType(memberPointerType(PointerToFunctionType)));

test/clang-tidy/modernize-redundant-void-arg.cpp

void lambda_expression_with_macro_test(){
   (void)WRAP([](void) {});
   // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant void argument list in lambda expression [modernize-redundant-void-arg]
   // CHECK-FIXES: (void)WRAP([]() {});
 
   [](void) BODY;
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant void argument list in lambda expression [modernize-redundant-void-arg]
   // CHECK-FIXES: []() BODY;
 }
+
+template <typename T0>
+struct S_template {
+  template <typename T>
+  void g_template() const {
+    int a_template;
+    (void)a_template;
+  }
+};
+
+void test_template() {
+  S_template<int>().g_template<int>();
+}
+