[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

Getting this false positive:

  #include 
  
  extern std::string str()
  {
std::string ret;
return ret.empty() ? ret : ret.substr(1);
  }



  input.cpp:6:23: warning: Parameter 'ret' is copied on last use, consider 
moving it instead. [performance-unnecessary-copy-on-last-use]
  return ret.empty() ? ret : ret.substr(1);
   ^
   std::move( )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

Another false positive:

  #include 
  
  void evaluateLibraryFunction()
  {
std::unordered_map m;
  
auto f = [m]() {};
  }



  input.cpp:7:12: warning: Parameter 'm' is copied on last use, consider moving 
it instead. [performance-unnecessary-copy-on-last-use]
  auto f = [m]() {};
^
std::move( )

This is suggested regardless of the C++ standard defined (unfortunately I 
didn't get a `-Wc++14-extensions` warning out of clang-tidy).

Also the fix-it will result in invalid code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

I am also experiencing a crash:

  #include 
  #include 
  
  class Token;
  
  class Value {
public:
using ErrorPathItem = std::pair;
using ErrorPath = std::list;
  
explicit Value(long long val = 0)
{}
  
Value(const Token* c, long long val);
  
ErrorPath errorPath; // removing this fixes the crash
  };
  
  void setTokenValue(Value value);
  
  template
  static void valueFlowForwardConst(const ContainerOfValue& values)
  {
[&] {
for (Value value : values) {
setTokenValue(value);
}
}();
  }



  Stack dump:
  0.  Program arguments: /mnt/s/GitHub/llvm-project/build/bin/clang-tidy 
-checks=-*,-clang-analyzer-*,performance-unnecessary-copy-on-last-use -fix 
/mnt/s/___temp/input.cpp
  1.   parser at end of file
  2.  ASTMatcher: Processing 'performance-unnecessary-copy-on-last-use' 
against:
  CXXConstructExpr : 
  --- Bound Nodes Begin ---
  constructExpr - { CXXConstructExpr :  }
  param - { DeclRefExpr :  }
  --- Bound Nodes End ---
  Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH 
or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamEi+0x23)[0x7f2fcfefa393]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN4llvm3sys17RunSignalHandlersEv+0xee)[0x7f2fcfef82ee]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3e3582f)[0x7f2fcfefa82f]
  /lib/x86_64-linux-gnu/libpthread.so.0(+0x14420)[0x7f2fcc06a420]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang4tidy11performance29UnnecessaryCopyOnLastUseCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0xfe)[0x7f2fcd598d5e]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffab94)[0x7f2fcf0bfb94]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang12ast_matchers8internal21BoundNodesTreeBuilder12visitMatchesEPNS2_7VisitorE+0x11c)[0x7f2fcf0ecdbc]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffa5be)[0x7f2fcf0bf5be]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3016505)[0x7f2fcf0db505]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x300b8a2)[0x7f2fcf0d08a2]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3009a73)[0x7f2fcf0cea73]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3009a73)[0x7f2fcf0cea73]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x30277c7)[0x7f2fcf0ec7c7]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3002c02)[0x7f2fcf0c7c02]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffd4f3)[0x7f2fcf0c24f3]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x30005b1)[0x7f2fcf0c55b1]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffd2cb)[0x7f2fcf0c22cb]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x300522b)[0x7f2fcf0ca22b]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffd7b1)[0x7f2fcf0c27b1]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang12ast_matchers11MatchFinder8matchASTERNS_10ASTContextE+0x357)[0x7f2fcf095f77]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang17MultiplexConsumer21HandleTranslationUnitERNS_10ASTContextE+0x2c)[0x7f2fce3dde8c]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang8ParseASTERNS_4SemaEbb+0x33f)[0x7f2fce5f4f5f]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang14FrontendAction7ExecuteEv+0x59)[0x7f2fce3a2799]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang16CompilerInstance13ExecuteActionERNS_14FrontendActionE+0x316)[0x7f2fce3151a6]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling21FrontendActionFactory13runInvocationESt10shared_ptrINS_18CompilerInvocationEEPNS_11FileManagerES2_INS_22PCHContainerOperationsEEPNS_18DiagnosticConsumerE+0x1ad)[0x7f2fcddc1d9d]
  /mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x1cc85e6)[0x7f2fcdd8d5e6]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling14ToolInvocation13runInvocationEPKcPNS_6driver11CompilationESt10shared_ptrINS_18CompilerInvocationEES7_INS_22PCHContainerOperationsEE+0x11a)[0x7f2fcddc1afa]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling14ToolInvocation3runEv+0x56c)[0x7f2fcddc092c]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling9ClangTool3runEPNS0_10ToolActionE+0xf9b)[0x7f2fcddc34ab]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang4tidy12runClangTidyERNS0_16ClangTidyContextERKNS_7tooling19CompilationDatabaseEN4llvm8ArrayRefINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcENS7_18IntrusiveRefCntPtrINS7_3vfs17OverlayFileSystemEEEbbNS7_9StringRefE+0x3f7)[0x7f2fcdd88ba7]
  
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang4tidy13clangTidyMainEiPPKc+0x2eaa)[0x7f2fcd0b9b8a]
  /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7f2fcbaa4083]
  /mnt/s/GitHub/l

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

The crash is gone.

The false positive with the `[m]` capture is still present with `-std=c++11`.

Here's another false positive:

  #include 
  
  void f(const std::string&);
  
  int main() {
  std::string s;
  f(s.empty() ? "<>" : s);
  }



  input.cpp:7:26: warning: Parameter 's' is copied on last use, consider moving 
it instead. [performance-unnecessary-copy-on-last-use]
  f(s.empty() ? "<>" : s);
   ^
   std::move( )

I still have another false positive with static variables but have not gotten 
around reducing it yet.

I posted the false negatives in the GitHub ticket as those do not relate to 
getting this approved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

In D137205#3937298 , @Febbe wrote:

> Does only a warning appear? Or also a fix? Currently, only a warning should 
> appear, but this is not fixable in c++11.

There's no fix-it. I didn't realize that before.

Maybe this should also be added to the documentation as a known limitation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

In D137205#3936944 , @firewave wrote:

> I still have another false positive with static variables but have not gotten 
> around reducing it yet.

Those false positives no longer appear. So no more open FPs from my side. Great 
work!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

Some additional thoughts I had a while ago about something I have raised before:
I think the warnings which can only be fixed with c++14 should either only be 
issued if that standard was specified or be behind an option. We have lots of 
lambda captures which could be moved with c++14 and having those warnings would 
lead to lots of suppressions within the code since we only target c++11.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Oliver Stöneberg via Phabricator via cfe-commits]

firewave added a comment.

In D137205#4225006 , @Febbe wrote:

> Yes, I agree, while I can't understand, why someone still wants to use only 
> c++11 I can totally understand, that a single Software Engineer can't do 
> anything about this, when the corporation does not permit it. 
> But it should not be removed, since this warning can still show some pitfalls 
> in performance critical code. A way in c++11 to fix the warning, is to create 
> a functor, instead of a lambda.

Well, compatibility with older platforms. And I personally have lots of 
quarrels with modern (and more recently even early) C++, but let's not get into 
that. Being able to move within captures is not one of them though...

I still think being able to tune that check for certain parts would be helpful 
- especially if it would not be fixable with a fix-it. It should also help with 
applying these findings to an existing code base as you could enable it 
incrementally (especially if you need to introduce something like functors).
Recent Clang additions like `misc-const-correctness` or `-Wunsafe-buffer-usage` 
would have profited from being just a bit more granular as they produce such a 
flood of warnings even for small code bases which makes it quite troublesome to 
adopt the code for those warnings as some of them are not just applying the 
fix-it but also need to be looked at in the bigger picture to see if they might 
not impact things negatively in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits