[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-26 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D52315#1245747, @dblaikie wrote: > ... I think this has been attempted to be superseded by https://reviews.llvm.org/D52360, although that has the same basic issue. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52315

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-25 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. "The three checks mentioned in the Title are two noisy if the code uses intrusive smart (reference counting) pointers. LLVM/Clang is such a code, it has lots of such types, e.g. StringRef, SymbolRef or ProgramStateRef, all of them based llvm::IntrusiveRefCntPtr. Every

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-21 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D52315#1241487, @baloghadamsoftware wrote: > I still wonder what true positives could we get without checking the size? No > real-life types come to my mind with size of a pointer but really expensive > copy operations. What could be so

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-21 Thread Balogh , Ádám via Phabricator via cfe-commits
baloghadamsoftware added a comment. I still wonder what true positives could we get without checking the size? No real-life types come to my mind with size of a pointer but really expensive copy operations. What could be so really expensive when copying a single machine world? Repository:

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-20 Thread Felix Berger via Phabricator via cfe-commits
flx added inline comments. Comment at: clang-tidy/utils/TypeTraits.cpp:49 + if (Context.getTypeSize(Type) <= Context.getTypeSize(Context.VoidPtrTy)) +return false; + This early return now ignores the fact that the type has non-trivial copy constructors,

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-20 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. This looks questionable to me. I don't disagree with the reasons stated about llvm types. But is that *always* true? I would personally be very surprized, and consider this a false-positive. This should at least be optional. Not sure about the default, but setting the

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-20 Thread Jonas Toth via Phabricator via cfe-commits
JonasToth added inline comments. Comment at: clang-tidy/utils/TypeTraits.cpp:47 + + // Do not consider "expensive to copy" types not greater than a pointer + if (Context.getTypeSize(Type) <= Context.getTypeSize(Context.VoidPtrTy)) Please make that comment a

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

2018-09-20 Thread Balogh , Ádám via Phabricator via cfe-commits
baloghadamsoftware created this revision. baloghadamsoftware added reviewers: alexfh, aaron.ballman, flx. The three checks mentioned in the Title are two noisy if the code uses intrusive smart (reference counting) pointers. LLVM/Clang is such a code, it has lots of such types, e.g. StringRef,