Page MenuHomePhabricator

[clang-tidy] misc-unused-parameters: Don't remove parameter from lambda
ClosedPublic

Authored by mgehre on Apr 7 2020, 2:41 PM.

Details

Summary

Previously, the check would fix

using fn = void(int);
void f(fn *);
void test() {
  // CHECK-MESSAGES: :[[@LINE+2]]:12: warning: parameter 'I' is unused
  // CHECK-FIXES: {{^}}  f([](int  /*I*/) {
  f([](int I) { return; });
}

into
f([]() { return; }); which breaks compilation. Now the check is disabled for lambdas.

The AST is not so easy to use. For

auto l = [](int) {  return;  };
f(l);

one gets

`-CallExpr <line:7:5, col:8> 'void'
     |-ImplicitCastExpr <col:5> 'void (*)(fn *)' <FunctionToPointerDecay>
     | `-DeclRefExpr <col:5> 'void (fn *)' lvalue Function 0x55a91a545e28 'f' 'void (fn *)'
     `-ImplicitCastExpr <col:7> 'void (*)(int)' <UserDefinedConversion>
       `-CXXMemberCallExpr <col:7> 'void (*)(int)'
         `-MemberExpr <col:7> '<bound member function type>' .operator void (*)(int) 0x55a91a546850
           `-ImplicitCastExpr <col:7> 'const (lambda at line:6:14)' lvalue <NoOp>
             `-DeclRefExpr <col:7> '(lambda at line:6:14)':'(lambda at line:6:14)' lvalue Var 0x55a91a5461c0 'l' '(lambda at line:6:14)':'(lambda at line:6:14)'

There is no direct use of the operator()(int I) of the lambda, so the !Indexer->getOtherRefs(Function).empty()
does not fire. In the future, we might be able to use the conversion operator operator void (*)(int) to mark
the call operator as having an "other ref".

Diff Detail

Event Timeline

mgehre created this revision.Apr 7 2020, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 2:41 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript
mgehre added a project: Restricted Project.Apr 7 2020, 2:42 PM
mgehre edited the summary of this revision. (Show Details)

Outright disabling for lambdas probably isn't the ideal solution, however it's at least a good stepping stone til a better solution can be found.

mgehre added a comment.Apr 8 2020, 5:20 AM

Outright disabling for lambdas probably isn't the ideal solution, however it's at least a good stepping stone til a better solution can be found.

I fully agree!

aaron.ballman accepted this revision.Apr 8 2020, 7:05 AM

LGTM as a temporary workaround until we find a better solution.

(I just noticed that we don't need a similar fix for blocks because this check doesn't seem to support blocks: https://godbolt.org/z/HZM2t2)

This revision is now accepted and ready to land.Apr 8 2020, 7:05 AM
This revision was automatically updated to reflect the committed changes.