llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.88k stars 11.93k forks source link

performance-unnecessary-value-param sometimes gets confused inside templates #99384

Open higher-performance opened 3 months ago

higher-performance commented 3 months ago
#include <vector>

template<class T = int>
void f(std::vector<int> v) {
  struct S {
    std::vector<T> v2;
  };
  auto a = S{
      .v2 = std::move(v),
  };
}

int main() {
    f(std::vector<int>());
}

[incorrectly triggers](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:12,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:1,endLineNumber:16,positionColumn:1,positionLineNumber:16,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:'%23include+%3Cvector%3E%0A%0Atemplate%3Cclass+T+%3D+int%3E%0Avoid+f(std::vector%3Cint%3E+v)+%7B%0A++struct+S+%7B%0A++++std::vector%3CT%3E+v2%3B%0A++%7D%3B%0A++auto+a+%3D+S%7B%0A++++++.v2+%3D+std::move(v),%0A++%7D%3B%0A%7D%0A%0Aint+main()+%7B%0A++++f(std::vector%3Cint%3E())%3B%0A%7D%0A'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:52.39869199990427,l:'4',m:100,n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'0',intel:'0',libraryCode:'0',trim:'0',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:12,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(trunk)+(Editor+%231)',t:'0')),k:48.77750596859633,l:'4',m:34.70312617491597,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+(trunk)',editorid:1,fontScale:12,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+(trunk)+(Compiler+%231)',t:'0')),header:(),l:'4',m:21.968034000634876,n:'0',o:'',s:0,t:'0'),(g:!((h:tool,i:(args:'-checks%3Dperformance-unnecessary-value-param',argsPanelShown:'0',compilerName:'x86-64+clang+(trunk)',editorid:1,fontScale:12,fontUsePx:'0',j:1,monacoEditorHasBeenAutoOpened:'1',monacoEditorOpen:'1',monacoStdin:'1',stdin:'',stdinPanelShown:'1',toolId:clangtidytrunk,wrap:'1'),l:'5',n:'0',o:'clang-tidy+(trunk)+x86-64+clang+(trunk)+(Editor+%231,+Compiler+%231)',t:'0')),l:'4',m:43.32883982444915,n:'0',o:'',s:0,t:'0')),k:47.601308000095734,l:'3',n:'0',o:'',t:'0')),l:'2',m:100,n:'0',o:'',t:'0')),version:4):

<source>:4:25: warning: the parameter 'v' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
    4 | void f(std::vector<int> v) {
      |                         ^
      |        const           &
1 warning generated.

despite the fact that it would not do so if vector<int> had been spelled vector<T>.

llvmbot commented 3 months ago

@llvm/issue-subscribers-clang-tidy

Author: None (higher-performance)

``` #include <vector> template<class T = int> void f(std::vector<int> v) { struct S { std::vector<T> v2; }; auto a = S{ .v2 = std::move(v), }; } int main() { f(std::vector<int>()); } ``` [incorrectly triggers](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:12,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:1,endLineNumber:16,positionColumn:1,positionLineNumber:16,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:'%23include+%3Cvector%3E%0A%0Atemplate%3Cclass+T+%3D+int%3E%0Avoid+f(std::vector%3Cint%3E+v)+%7B%0A++struct+S+%7B%0A++++std::vector%3CT%3E+v2%3B%0A++%7D%3B%0A++auto+a+%3D+S%7B%0A++++++.v2+%3D+std::move(v),%0A++%7D%3B%0A%7D%0A%0Aint+main()+%7B%0A++++f(std::vector%3Cint%3E())%3B%0A%7D%0A'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:52.39869199990427,l:'4',m:100,n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'0',intel:'0',libraryCode:'0',trim:'0',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:12,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(trunk)+(Editor+%231)',t:'0')),k:48.77750596859633,l:'4',m:34.70312617491597,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+(trunk)',editorid:1,fontScale:12,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+(trunk)+(Compiler+%231)',t:'0')),header:(),l:'4',m:21.968034000634876,n:'0',o:'',s:0,t:'0'),(g:!((h:tool,i:(args:'-checks%3Dperformance-unnecessary-value-param',argsPanelShown:'0',compilerName:'x86-64+clang+(trunk)',editorid:1,fontScale:12,fontUsePx:'0',j:1,monacoEditorHasBeenAutoOpened:'1',monacoEditorOpen:'1',monacoStdin:'1',stdin:'',stdinPanelShown:'1',toolId:clangtidytrunk,wrap:'1'),l:'5',n:'0',o:'clang-tidy+(trunk)+x86-64+clang+(trunk)+(Editor+%231,+Compiler+%231)',t:'0')),l:'4',m:43.32883982444915,n:'0',o:'',s:0,t:'0')),k:47.601308000095734,l:'3',n:'0',o:'',t:'0')),l:'2',m:100,n:'0',o:'',t:'0')),version:4): ``` <source>:4:25: warning: the parameter 'v' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] 4 | void f(std::vector<int> v) { | ^ | const & 1 warning generated. ``` despite the fact that it would not do so if `vector<int>` had been spelled `vector<T>`.
PiotrZSL commented 3 months ago

There is a chance that this commit fixes it: 1ce89899ad33a0d2976859d8d278dba4342cbb6b

j-piecuch commented 1 month ago

Stumbled on this false positive too, here's my reproducer:

#include <string>
#include <type_traits>
#include <utility>

struct Pair {
  bool b = false;
  std::string s;
};

template <typename T>
static Pair Create(std::string str) {
  return Pair{.b = std::is_integral_v<T>, .s = std::move(str)};
};

Warning generated:

repro.cc:11:32: warning: the parameter 'str' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
   11 | static Pair Create(std::string str) {
      |                                ^
      |                    const      &

Interestingly, if I don't use a designated initializer, there's no warning:

template <typename T>
static Pair Create(std::string str) {
  return Pair{std::is_integral_v<T>, std::move(str)};
};
j-piecuch commented 1 month ago

Oh, and forgot to add: https://github.com/llvm/llvm-project/commit/1ce89899ad33a0d2976859d8d278dba4342cbb6b fixes neither my case nor the one from the original comment.