Skip to content

clang-tidy: performance-unnecessary-value-param #19870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 3, 2019
Merged

clang-tidy: performance-unnecessary-value-param #19870

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4c72a3
clang-tidy: performance-unnecessary-value-param
JohnCoconut Jun 26, 2019
e125dab
Reference: https://releases.llvm.org/7.0.0/tools/clang/tools/extra/do…
JohnCoconut Jun 27, 2019
5308da4
perfer pass by const reference
JohnCoconut Jul 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Checks: >
performance-move-constructor-init,
performance-type-promotion-in-math-fn,
performance-unnecessary-copy-initialization,
performance-unnecessary-value-param,
readability-container-size-empty,
readability-delete-null-pointer,
readability-redundant-control-flow,
Expand Down
6 changes: 4 additions & 2 deletions cocos/2d/CCActionInterval.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ THE SOFTWARE.

#include <stdarg.h>

#include <utility>

#include "2d/CCSprite.h"
#include "2d/CCNode.h"
#include "2d/CCSpriteFrame.h"
Expand Down Expand Up @@ -2803,7 +2805,7 @@ void TargetedAction::setForcedTarget(Node* forcedTarget)
ActionFloat* ActionFloat::create(float duration, float from, float to, ActionFloatCallback callback)
{
auto ref = new (std::nothrow) ActionFloat();
if (ref && ref->initWithDuration(duration, from, to, callback))
if (ref && ref->initWithDuration(duration, from, to, std::move(callback)))
{
ref->autorelease();
return ref;
Expand All @@ -2819,7 +2821,7 @@ bool ActionFloat::initWithDuration(float duration, float from, float to, ActionF
{
_from = from;
_to = to;
_callback = callback;
_callback = std::move(callback);
return true;
}
return false;
Expand Down
6 changes: 3 additions & 3 deletions cocos/2d/CCNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ Node* Node::getChildByName(const std::string& name) const
return nullptr;
}

void Node::enumerateChildren(const std::string &name, std::function<bool (Node *)> callback) const
void Node::enumerateChildren(const std::string &name, const std::function<bool (Node *)>& callback) const
{
CCASSERT(!name.empty(), "Invalid name");
CCASSERT(callback != nullptr, "Invalid callback function");
Expand Down Expand Up @@ -869,7 +869,7 @@ void Node::enumerateChildren(const std::string &name, std::function<bool (Node *
}
}

bool Node::doEnumerateRecursive(const Node* node, const std::string &name, std::function<bool (Node *)> callback) const
bool Node::doEnumerateRecursive(const Node* node, const std::string &name, const std::function<bool (Node *)>& callback) const
{
bool ret =false;

Expand All @@ -894,7 +894,7 @@ bool Node::doEnumerateRecursive(const Node* node, const std::string &name, std::
return ret;
}

bool Node::doEnumerate(std::string name, std::function<bool (Node *)> callback) const
bool Node::doEnumerate(std::string name, const std::function<bool (Node *)>& callback) const
{
// name may be xxx/yyy, should find its parent
size_t pos = name.find('/');
Expand Down
6 changes: 3 additions & 3 deletions cocos/2d/CCNode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ class CC_DLL Node : public Ref
*
* @since v3.2
*/
virtual void enumerateChildren(const std::string &name, std::function<bool(Node* node)> callback) const;
virtual void enumerateChildren(const std::string &name, const std::function<bool(Node* node)>& callback) const;
/**
* Returns the array of the node's children.
*
Expand Down Expand Up @@ -1886,8 +1886,8 @@ class CC_DLL Node : public Ref
virtual void disableCascadeColor();
virtual void updateColor() {}

bool doEnumerate(std::string name, std::function<bool (Node *)> callback) const;
bool doEnumerateRecursive(const Node* node, const std::string &name, std::function<bool (Node *)> callback) const;
bool doEnumerate(std::string name, const std::function<bool (Node *)>& callback) const;
bool doEnumerateRecursive(const Node* node, const std::string &name, const std::function<bool (Node *)>& callback) const;

//check whether this camera mask is visible by the current visiting camera
bool isVisitableByVisitingCamera() const;
Expand Down
10 changes: 6 additions & 4 deletions cocos/2d/CCRenderTexture.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ THE SOFTWARE.

#include "2d/CCRenderTexture.h"

#include <utility>

#include "base/ccUtils.h"
#include "platform/CCFileUtils.h"
#include "base/CCEventType.h"
Expand Down Expand Up @@ -506,7 +508,7 @@ void RenderTexture::visit(Renderer *renderer, const Mat4 &parentTransform, uint3
// setOrderOfArrival(0);
}

bool RenderTexture::saveToFileAsNonPMA(const std::string& filename, bool isRGBA, std::function<void(RenderTexture*, const std::string&)> callback)
bool RenderTexture::saveToFileAsNonPMA(const std::string& filename, bool isRGBA, const std::function<void(RenderTexture*, const std::string&)>& callback)
{
std::string basename(filename);
std::transform(basename.begin(), basename.end(), basename.begin(), ::tolower);
Expand All @@ -528,7 +530,7 @@ bool RenderTexture::saveToFileAsNonPMA(const std::string& filename, bool isRGBA,
return saveToFileAsNonPMA(filename, Image::Format::JPG, false, callback);
}

bool RenderTexture::saveToFile(const std::string& filename, bool isRGBA, std::function<void (RenderTexture*, const std::string&)> callback)
bool RenderTexture::saveToFile(const std::string& filename, bool isRGBA, const std::function<void (RenderTexture*, const std::string&)>& callback)
{
std::string basename(filename);
std::transform(basename.begin(), basename.end(), basename.begin(), ::tolower);
Expand Down Expand Up @@ -556,7 +558,7 @@ bool RenderTexture::saveToFileAsNonPMA(const std::string& fileName, Image::Forma
"the image can only be saved as JPG or PNG format");
if (isRGBA && format == Image::Format::JPG) CCLOG("RGBA is not supported for JPG format");

_saveFileCallback = callback;
_saveFileCallback = std::move(callback);

std::string fullpath = FileUtils::getInstance()->getWritablePath() + fileName;
_saveToFileCommand.init(_globalZOrder);
Expand All @@ -572,7 +574,7 @@ bool RenderTexture::saveToFile(const std::string& fileName, Image::Format format
"the image can only be saved as JPG or PNG format");
if (isRGBA && format == Image::Format::JPG) CCLOG("RGBA is not supported for JPG format");

_saveFileCallback = callback;
_saveFileCallback = std::move(callback);

std::string fullpath = FileUtils::getInstance()->getWritablePath() + fileName;
_saveToFileCommand.init(_globalZOrder);
Expand Down
4 changes: 2 additions & 2 deletions cocos/2d/CCRenderTexture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class CC_DLL RenderTexture : public Node
* @param callback When the file is save finished,it will callback this function.
* @return Returns true if the operation is successful.
*/
bool saveToFileAsNonPMA(const std::string& filename, bool isRGBA = true, std::function<void(RenderTexture*, const std::string&)> callback = nullptr);
bool saveToFileAsNonPMA(const std::string& filename, bool isRGBA = true, const std::function<void(RenderTexture*, const std::string&)>& callback = nullptr);


/** Saves the texture into a file using JPEG format. The file will be saved in the Documents folder.
Expand All @@ -170,7 +170,7 @@ class CC_DLL RenderTexture : public Node
* @param callback When the file is save finished,it will callback this function.
* @return Returns true if the operation is successful.
*/
bool saveToFile(const std::string& filename, bool isRGBA = true, std::function<void (RenderTexture*, const std::string&)> callback = nullptr);
bool saveToFile(const std::string& filename, bool isRGBA = true, const std::function<void (RenderTexture*, const std::string&)>& callback = nullptr);

/** saves the texture into a file in non-PMA. The format could be JPG or PNG. The file will be saved in the Documents folder.
Returns true if the operation is successful.
Expand Down
9 changes: 5 additions & 4 deletions cocos/2d/CCTMXXMLParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@ THE SOFTWARE.
****************************************************************************/

#include "2d/CCTMXXMLParser.h"
#include <unordered_map>
#include <sstream>
#include "2d/CCTMXTiledMap.h"
#include "base/CCDirector.h"
#include "base/ZipUtils.h"
#include "base/base64.h"
#include "base/CCDirector.h"
#include "platform/CCFileUtils.h"
#include <sstream>
#include <unordered_map>
#include <utility>

using namespace std;

Expand Down Expand Up @@ -65,7 +66,7 @@ ValueMap& TMXLayerInfo::getProperties()

void TMXLayerInfo::setProperties(ValueMap var)
{
_properties = var;
_properties = std::move(var);
}

// implementation TMXTilesetInfo
Expand Down
2 changes: 1 addition & 1 deletion cocos/3d/CCTerrain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ void Terrain::setAlphaMap(cocos2d::Texture2D * newAlphaMapTexture)
_alphaMap = newAlphaMapTexture;
}

void Terrain::setDetailMap(unsigned int index, DetailMap detailMap)
void Terrain::setDetailMap(unsigned int index, const DetailMap& detailMap)
{
if(index>4)
{
Expand Down
2 changes: 1 addition & 1 deletion cocos/3d/CCTerrain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ class CC_DLL Terrain : public Node
/** set the alpha map*/
void setAlphaMap(cocos2d::Texture2D * newAlphaMapTexture);
/**set the Detail Map */
void setDetailMap(unsigned int index, DetailMap detailMap);
void setDetailMap(unsigned int index, const DetailMap& detailMap);

// Overrides, internal use only
virtual void draw(cocos2d::Renderer* renderer, const cocos2d::Mat4 &transform, uint32_t flags) override;
Expand Down
2 changes: 1 addition & 1 deletion cocos/audio/AudioEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ AudioProfile* AudioEngine::getProfile(const std::string &name)
}
}

void AudioEngine::preload(const std::string& filePath, std::function<void(bool isSuccess)> callback)
void AudioEngine::preload(const std::string& filePath, const std::function<void(bool isSuccess)>& callback)
{
if (!isEnabled())
{
Expand Down
2 changes: 1 addition & 1 deletion cocos/audio/include/AudioEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ class EXPORT_DLL AudioEngine
* @param filePath The file path of an audio.
* @param callback A callback which will be called after loading is finished.
*/
static void preload(const std::string& filePath, std::function<void(bool isSuccess)> callback);
static void preload(const std::string& filePath, const std::function<void(bool isSuccess)>& callback);

/**
* Gets playing audio count.
Expand Down
2 changes: 1 addition & 1 deletion cocos/audio/linux/AudioEngine-linux.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ void AudioEngineImpl::uncacheAll()
mapId.clear();
}

int AudioEngineImpl::preload(const std::string& filePath, std::function<void(bool isSuccess)> callback)
int AudioEngineImpl::preload(const std::string& filePath, const std::function<void(bool isSuccess)>& callback)
{
FMOD::Sound * sound = findSound(filePath);
if (!sound) {
Expand Down
2 changes: 1 addition & 1 deletion cocos/audio/linux/AudioEngine-linux.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class CC_DLL AudioEngineImpl : public cocos2d::Ref
void uncacheAll();


int preload(const std::string& filePath, std::function<void(bool isSuccess)> callback);
int preload(const std::string& filePath, const std::function<void(bool isSuccess)>& callback);

void update(float dt);

Expand Down
5 changes: 3 additions & 2 deletions cocos/base/ObjectFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
****************************************************************************/

#include <functional>
#include "base/ObjectFactory.h"
#include <functional>
#include <utility>


NS_CC_BEGIN
Expand All @@ -47,7 +48,7 @@ ObjectFactory::TInfo::TInfo(const std::string& type, Instance ins)
ObjectFactory::TInfo::TInfo(const std::string& type, InstanceFunc ins)
:_class(type)
,_fun(nullptr)
,_func(ins)
,_func(std::move(ins))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use const InstanceFunc &ins instead of std::move here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read e.g.:
https://abseil.io/tips/117
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#exception-9

Personally I think that const& should be used here. This is what is recommended here: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f15-prefer-simple-and-conventional-ways-of-passing-information

And later if optimization is needed then pass by && or "perfect forwarding".

So personally I do not like "pass by value and then move" but some C++ experts and clang-tidy claim that's OK.

Change to "const&" will also fix this warning but there is another check for storing temporary variables which should be fixed with:
"pass by &&" or "perfect forwarding" or "pass by value+std::move"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to "const&" will also fix this warning but there is another check for storing temporary variables which should be fixed with:
"pass by &&" or "perfect forwarding" or "pass by value+std::move"

I don't quite understand why this is a warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InstanceFunc is std:function. std::function could be expensive to copy (e.g. when lamda capture big object std::function do allocation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is copied when passing by value, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if you call like that:

f(std::move(instanceFun));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's copied once when passed to constructor.

ObjectFactory::TInfo::TInfo(const std::string& type, InstanceFunc ins)

It will be copied again in member intializer.

,_func(ins)

I have a example here. https://wandbox.org/permlink/PXqmQmvCgdzvEFNF

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the same without copy but with moves:
https://wandbox.org/permlink/dW0KTViJhS9YNdnz

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazyhappygame yep, in your test case, it is not copied, but the invoker should pass using std::move(). Which means the invoker should be very careful to use it, if passing by reference, then invoker don't have to take care of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I prefer pass by const & (and than pass by && if needed)

I think that there is another clang-tidy to check if std::move is used where needed https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html

{
ObjectFactory::getInstance()->registerType(*this);
}
Expand Down
4 changes: 2 additions & 2 deletions cocos/editor-support/cocosbuilder/CCBAnimationManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ void CCBAnimationManager::addDocumentCallbackNode(Node *node)
_documentCallbackNodes.pushBack(node);
}

void CCBAnimationManager::addDocumentCallbackName(std::string name)
void CCBAnimationManager::addDocumentCallbackName(const std::string& name)
{
_documentCallbackNames.push_back(Value(name));
}
Expand Down Expand Up @@ -170,7 +170,7 @@ void CCBAnimationManager::addDocumentOutletNode(Node *node)
_documentOutletNodes.pushBack(node);
}

void CCBAnimationManager::addDocumentOutletName(std::string name)
void CCBAnimationManager::addDocumentOutletName(const std::string& name)
{
_documentOutletNames.push_back(Value(name));
}
Expand Down
4 changes: 2 additions & 2 deletions cocos/editor-support/cocosbuilder/CCBAnimationManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ class CC_DLL CCBAnimationManager : public cocos2d::Ref


void addDocumentCallbackNode(cocos2d::Node *node);
void addDocumentCallbackName(std::string name);
void addDocumentCallbackName(const std::string& name);
void addDocumentCallbackControlEvents(cocos2d::extension::Control::EventType eventType);

void addDocumentOutletNode(cocos2d::Node *node);
void addDocumentOutletName(std::string name);
void addDocumentOutletName(const std::string& name);

void setDocumentControllerName(const std::string &name);

Expand Down
19 changes: 10 additions & 9 deletions cocos/editor-support/cocosbuilder/CCBReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,17 @@
#include "2d/CCSpriteFrameCache.h"
#include "renderer/CCTextureCache.h"

#include "editor-support/cocosbuilder/CCBAnimationManager.h"
#include "editor-support/cocosbuilder/CCBKeyframe.h"
#include "editor-support/cocosbuilder/CCBMemberVariableAssigner.h"
#include "editor-support/cocosbuilder/CCBReader.h"
#include "editor-support/cocosbuilder/CCBSelectorResolver.h"
#include "editor-support/cocosbuilder/CCBSequenceProperty.h"
#include "editor-support/cocosbuilder/CCNodeLoader.h"
#include "editor-support/cocosbuilder/CCNodeLoaderLibrary.h"
#include "editor-support/cocosbuilder/CCNodeLoaderListener.h"
#include "editor-support/cocosbuilder/CCBMemberVariableAssigner.h"
#include "editor-support/cocosbuilder/CCBSelectorResolver.h"
#include "editor-support/cocosbuilder/CCBAnimationManager.h"
#include "editor-support/cocosbuilder/CCBSequenceProperty.h"
#include "editor-support/cocosbuilder/CCBKeyframe.h"
#include <sstream>
#include <utility>

using namespace cocos2d;
using namespace cocos2d::extension;
Expand Down Expand Up @@ -193,7 +194,7 @@ CCBReader::CCBAnimationManagerMapPtr CCBReader::getAnimationManagers()

void CCBReader::setAnimationManagers(CCBAnimationManagerMapPtr x)
{
_animationManagers = x;
_animationManagers = std::move(x);
}

CCBMemberVariableAssigner * CCBReader::getCCBMemberVariableAssigner() {
Expand Down Expand Up @@ -255,7 +256,7 @@ Node* CCBReader::readNodeGraphFromFile(const char *pCCBFileName, Ref *pOwner, co

Node* CCBReader::readNodeGraphFromData(std::shared_ptr<cocos2d::Data> data, Ref *pOwner, const Size &parentSize)
{
_data = data;
_data = std::move(data);
_bytes =_data->getBytes();
_currentByte = 0;
_currentBit = 0;
Expand Down Expand Up @@ -337,7 +338,7 @@ Node* CCBReader::readFileWithCleanUp(bool bCleanUp, CCBAnimationManagerMapPtr am
return nullptr;
}

setAnimationManagers(am);
setAnimationManagers(std::move(am));

Node *pNode = readNodeGraph(nullptr);

Expand Down Expand Up @@ -1069,7 +1070,7 @@ Vector<CCBAnimationManager*>& CCBReader::getAnimationManagersForNodes()
return _animationManagersForNodes;
}

void CCBReader::addOwnerOutletName(std::string name)
void CCBReader::addOwnerOutletName(const std::string& name)
{
_ownerOutletNames.push_back(name);
}
Expand Down
2 changes: 1 addition & 1 deletion cocos/editor-support/cocosbuilder/CCBReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ class CC_DLL CCBReader : public cocos2d::Ref
*/
cocos2d::Node* readFileWithCleanUp(bool bCleanUp, CCBAnimationManagerMapPtr am);

void addOwnerOutletName(std::string name);
void addOwnerOutletName(const std::string& name);
void addOwnerOutletNode(cocos2d::Node *node);

private:
Expand Down
Loading