This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by IdrissRio on Sep 15 2018, 7:21 AM.

Details

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;
 ^~~~

Event Timeline

IdrissRio created this revision.Sep 15 2018, 7:21 AM
IdrissRio retitled this revision from 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. to [Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about variable cast to void.Sep 15 2018, 7:22 AM
IdrissRio edited the summary of this revision. (Show Details)
IdrissRio edited the summary of this revision. (Show Details)Sep 15 2018, 7:25 AM

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.

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.

Eugene.Zelenko added a project: Restricted Project.Sep 15 2018, 8:38 AM
JonasToth added a subscriber: JonasToth.EditedSep 16 2018, 9:18 AM

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 updated this revision to Diff 165683.EditedSep 16 2018, 10:24 AM
IdrissRio edited the summary of this revision. (Show Details)

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.

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.

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 updated this revision to Diff 165741.Sep 17 2018, 4:39 AM

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

IdrissRio marked an inline comment as done.Sep 17 2018, 4:40 AM
JonasToth accepted this revision.Sep 17 2018, 4:42 AM

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.

This revision is now accepted and ready to land.Sep 17 2018, 4:42 AM
JonasToth added inline comments.Sep 17 2018, 4:43 AM
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.

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.

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 accepted this revision.Sep 17 2018, 4:52 AM

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

IdrissRio marked 2 inline comments as done.Sep 17 2018, 5:29 AM
This revision was automatically updated to reflect the committed changes.