Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Turned on performance-unnecessary-value-param everywhere. #37447

Merged
merged 7 commits into from
Nov 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ clang-analyzer-*,\
readability-identifier-naming,\
clang-diagnostic-*,\
google-objc-*,\
google-explicit-constructor"
google-explicit-constructor,\
performance-unnecessary-value-param"

CheckOptions:
- key: modernize-use-default-member-init.UseAssignment
Expand Down
2 changes: 1 addition & 1 deletion ci/lint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ DART_BIN="${SRC_DIR}/third_party/dart/tools/sdks/dart-sdk/bin"
DART="${DART_BIN}/dart"
# TODO(https://github.com/flutter/flutter/issues/113848): Migrate all platforms
# to have this as an error.
MAC_HOST_WARNINGS_AS_ERRORS="performance-move-const-arg,performance-unnecessary-value-param"
MAC_HOST_WARNINGS_AS_ERRORS="performance-move-const-arg"

# FLUTTER_LINT_PRINT_FIX will make it so that fix is executed and the generated
# diff is printed to stdout if clang-tidy fails. This is helpful for enabling
Expand Down
6 changes: 4 additions & 2 deletions fml/platform/android/jni_weak_ref.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,14 @@ ScopedJavaLocalRef<jobject> GetRealObject(JNIEnv* env, jweak obj) {
}

void JavaObjectWeakGlobalRef::Assign(const JavaObjectWeakGlobalRef& other) {
if (&other == this)
if (&other == this) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there are a few stray flags that are not turned on as errors still.

return;
}

JNIEnv* env = AttachCurrentThread();
if (obj_)
if (obj_) {
env->DeleteWeakGlobalRef(obj_);
}

obj_ = other.obj_ ? env->NewWeakGlobalRef(other.obj_) : NULL;
}
Expand Down
6 changes: 4 additions & 2 deletions impeller/renderer/backend/vulkan/command_buffer_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "impeller/renderer/backend/vulkan/command_buffer_vk.h"

#include <utility>

#include "flutter/fml/logging.h"
#include "impeller/base/validation.h"
#include "impeller/renderer/backend/vulkan/context_vk.h"
Expand All @@ -15,7 +17,7 @@
namespace impeller {

std::shared_ptr<CommandBufferVK> CommandBufferVK::Create(
std::weak_ptr<const Context> context,
const std::weak_ptr<const Context>& context,
vk::Device device,
vk::CommandPool command_pool,
SurfaceProducerVK* surface_producer) {
Expand All @@ -41,7 +43,7 @@ CommandBufferVK::CommandBufferVK(std::weak_ptr<const Context> context,
SurfaceProducerVK* surface_producer,
vk::CommandPool command_pool,
vk::UniqueCommandBuffer command_buffer)
: CommandBuffer(context),
: CommandBuffer(std::move(context)),
Copy link
Member Author

Choose a reason for hiding this comment

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

Things like this will get wiped out when I turn on the const move linter.

device_(device),
command_pool_(command_pool),
command_buffer_(std::move(command_buffer)),
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/command_buffer_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace impeller {
class CommandBufferVK final : public CommandBuffer {
public:
static std::shared_ptr<CommandBufferVK> Create(
std::weak_ptr<const Context> context,
const std::weak_ptr<const Context>& context,
vk::Device device,
vk::CommandPool command_pool,
SurfaceProducerVK* surface_producer);
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/device_buffer_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ DeviceBufferVK::DeviceBufferVK(
DeviceBufferDescriptor desc,
ContextVK& context,
std::unique_ptr<DeviceBufferAllocationVK> device_allocation)
: DeviceBuffer(std::move(desc)),
: DeviceBuffer(desc),
context_(context),
device_allocation_(std::move(device_allocation)) {}

Expand Down
4 changes: 2 additions & 2 deletions impeller/renderer/backend/vulkan/pipeline_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ vk::DescriptorSetLayout PipelineCreateInfoVK::GetDescriptorSetLayout() const {
}

PipelineVK::PipelineVK(std::weak_ptr<PipelineLibrary> library,
PipelineDescriptor desc,
const PipelineDescriptor& desc,
std::unique_ptr<PipelineCreateInfoVK> create_info)
: Pipeline(std::move(library), std::move(desc)),
: Pipeline(std::move(library), desc),
pipeline_info_(std::move(create_info)) {}

PipelineVK::~PipelineVK() = default;
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/pipeline_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class PipelineVK final
public BackendCast<PipelineVK, Pipeline<PipelineDescriptor>> {
public:
PipelineVK(std::weak_ptr<PipelineLibrary> library,
PipelineDescriptor desc,
const PipelineDescriptor& desc,
std::unique_ptr<PipelineCreateInfoVK> create_info);

// |Pipeline|
Expand Down
5 changes: 3 additions & 2 deletions impeller/renderer/backend/vulkan/surface_producer_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "impeller/renderer/backend/vulkan/surface_producer_vk.h"

#include <array>
#include <utility>

#include "impeller/base/validation.h"
#include "impeller/renderer/backend/vulkan/surface_vk.h"
Expand All @@ -13,7 +14,7 @@
namespace impeller {

std::unique_ptr<SurfaceProducerVK> SurfaceProducerVK::Create(
std::weak_ptr<Context> context,
const std::weak_ptr<Context>& context,
const SurfaceProducerCreateInfoVK& create_info) {
auto surface_producer =
std::make_unique<SurfaceProducerVK>(context, create_info);
Expand All @@ -28,7 +29,7 @@ std::unique_ptr<SurfaceProducerVK> SurfaceProducerVK::Create(
SurfaceProducerVK::SurfaceProducerVK(
std::weak_ptr<Context> context,
const SurfaceProducerCreateInfoVK& create_info)
: context_(context), create_info_(create_info) {}
: context_(std::move(context)), create_info_(create_info) {}

SurfaceProducerVK::~SurfaceProducerVK() = default;

Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/surface_producer_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class SurfaceSyncObjectsVK {
class SurfaceProducerVK {
public:
static std::unique_ptr<SurfaceProducerVK> Create(
std::weak_ptr<Context> context,
const std::weak_ptr<Context>& context,
const SurfaceProducerCreateInfoVK& create_info);

SurfaceProducerVK(std::weak_ptr<Context> context,
Expand Down
11 changes: 5 additions & 6 deletions impeller/renderer/backend/vulkan/surface_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ std::unique_ptr<SurfaceVK> SurfaceVK::WrapSwapchainImage(
.frame_num = frame_num,
},
});
color0.texture = std::make_shared<TextureVK>(std::move(color0_tex), context,
color0.texture = std::make_shared<TextureVK>(color0_tex, context,
std::move(color_texture_info));
color0.clear_color = Color::DarkSlateGray();
color0.load_action = LoadAction::kClear;
Expand All @@ -59,19 +59,18 @@ std::unique_ptr<SurfaceVK> SurfaceVK::WrapSwapchainImage(
},
});
stencil0.texture = std::make_shared<TextureVK>(
std::move(stencil0_tex), context, std::move(stencil_texture_info));
stencil0_tex, context, std::move(stencil_texture_info));
stencil0.load_action = LoadAction::kClear;
stencil0.store_action = StoreAction::kDontCare;

RenderTarget render_target_desc;
render_target_desc.SetColorAttachment(color0, 0u);

return std::unique_ptr<SurfaceVK>(new SurfaceVK(std::move(render_target_desc),
swapchain_image,
std::move(swap_callback)));
return std::unique_ptr<SurfaceVK>(new SurfaceVK(
render_target_desc, swapchain_image, std::move(swap_callback)));
}

SurfaceVK::SurfaceVK(RenderTarget target,
SurfaceVK::SurfaceVK(const RenderTarget& target,
SwapchainImageVK* swapchain_image,
SwapCallback swap_callback)
: Surface(target) {
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/surface_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class SurfaceVK final : public Surface {
ContextVK* context,
SwapCallback swap_callback);

SurfaceVK(RenderTarget target,
SurfaceVK(const RenderTarget& target,
SwapchainImageVK* swapchain_image,
SwapCallback swap_callback);

Expand Down
2 changes: 1 addition & 1 deletion impeller/toolkit/egl/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace impeller {
namespace egl {

Config::Config(ConfigDescriptor descriptor, EGLConfig config)
: desc_(std::move(descriptor)), config_(config) {}
: desc_(descriptor), config_(config) {}

Config::~Config() = default;

Expand Down
2 changes: 1 addition & 1 deletion impeller/toolkit/egl/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ bool Context::ClearCurrent() const {
}

std::optional<UniqueID> Context::AddLifecycleListener(
LifecycleListener listener) {
const LifecycleListener& listener) {
if (!listener) {
return std::nullopt;
}
Expand Down
3 changes: 2 additions & 1 deletion impeller/toolkit/egl/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ class Context {
kWillClearCurrent,
};
using LifecycleListener = std::function<void(LifecycleEvent)>;
std::optional<UniqueID> AddLifecycleListener(LifecycleListener listener);
std::optional<UniqueID> AddLifecycleListener(
const LifecycleListener& listener);

bool RemoveLifecycleListener(UniqueID id);

Expand Down
2 changes: 1 addition & 1 deletion impeller/toolkit/egl/display.cc
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ std::unique_ptr<Config> Display::ChooseConfig(ConfigDescriptor config) const {
return nullptr;
}

return std::make_unique<Config>(std::move(config), config_out);
return std::make_unique<Config>(config, config_out);
}

std::unique_ptr<Surface> Display::CreateWindowSurface(
Expand Down
5 changes: 3 additions & 2 deletions lib/ui/ui_dart_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,16 @@ void UIDartState::LogMessage(const std::string& tag,
// Fall back to previous behavior if unspecified.
#if defined(FML_OS_ANDROID)
__android_log_print(ANDROID_LOG_INFO, tag.c_str(), "%.*s",
(int)message.size(), message.c_str());
static_cast<int>(message.size()), message.c_str());
#elif defined(FML_OS_IOS)
std::stringstream stream;
if (!tag.empty()) {
stream << tag << ": ";
}
stream << message;
std::string log = stream.str();
syslog(1 /* LOG_ALERT */, "%.*s", (int)log.size(), log.c_str());
syslog(1 /* LOG_ALERT */, "%.*s", static_cast<int>(log.size()),
log.c_str());
#else
if (!tag.empty()) {
std::cout << tag << ": ";
Expand Down
2 changes: 1 addition & 1 deletion shell/common/rasterizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ std::unique_ptr<Rasterizer::GpuImageResult> Rasterizer::MakeSkiaGpuImage(
// I can't seem to get the GPU path working on it.
// https://github.com/flutter/flutter/issues/108835
#if FML_OS_LINUX
return MakeBitmapImage(std::move(display_list), image_info);
return MakeBitmapImage(display_list, image_info);
#endif

std::unique_ptr<SnapshotDelegate::GpuImageResult> result;
Expand Down
2 changes: 1 addition & 1 deletion shell/common/shell_io_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace flutter {

sk_sp<GrDirectContext> ShellIOManager::CreateCompatibleResourceLoadingContext(
GrBackend backend,
sk_sp<const GrGLInterface> gl_interface) {
const sk_sp<const GrGLInterface>& gl_interface) {
#if SK_GL
if (backend != GrBackend::kOpenGL_GrBackend) {
return nullptr;
Expand Down
2 changes: 1 addition & 1 deletion shell/common/shell_io_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class ShellIOManager final : public IOManager {
// they so desire.
static sk_sp<GrDirectContext> CreateCompatibleResourceLoadingContext(
GrBackend backend,
sk_sp<const GrGLInterface> gl_interface);
const sk_sp<const GrGLInterface>& gl_interface);

ShellIOManager(
sk_sp<GrDirectContext> resource_context,
Expand Down
4 changes: 2 additions & 2 deletions shell/platform/android/android_context_gl_skia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ AndroidContextGLSkia::AndroidContextGLSkia(
const TaskRunners& task_runners,
uint8_t msaa_samples)
: AndroidContext(AndroidRenderingAPI::kOpenGLES),
environment_(environment),
environment_(std::move(environment)),
config_(nullptr),
task_runners_(task_runners) {
if (!environment_->IsValid()) {
Expand Down Expand Up @@ -146,7 +146,7 @@ AndroidContextGLSkia::~AndroidContextGLSkia() {
}

std::unique_ptr<AndroidEGLSurface> AndroidContextGLSkia::CreateOnscreenSurface(
fml::RefPtr<AndroidNativeWindow> window) const {
const fml::RefPtr<AndroidNativeWindow>& window) const {
if (window->IsFakeWindow()) {
return CreatePbufferSurface();
} else {
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/android_context_gl_skia.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class AndroidContextGLSkia : public AndroidContext {
/// @return The window surface.
///
std::unique_ptr<AndroidEGLSurface> CreateOnscreenSurface(
fml::RefPtr<AndroidNativeWindow> window) const;
const fml::RefPtr<AndroidNativeWindow>& window) const;

//----------------------------------------------------------------------------
/// @brief Allocates an 1x1 pbuffer surface that is used for making the
Expand Down
4 changes: 3 additions & 1 deletion shell/platform/android/android_external_texture_gl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include <GLES/glext.h>

#include <utility>

#include "third_party/skia/include/core/SkAlphaType.h"
#include "third_party/skia/include/core/SkColorSpace.h"
#include "third_party/skia/include/core/SkColorType.h"
Expand All @@ -20,7 +22,7 @@ AndroidExternalTextureGL::AndroidExternalTextureGL(
const fml::jni::ScopedJavaGlobalRef<jobject>& surface_texture,
std::shared_ptr<PlatformViewAndroidJNI> jni_facade)
: Texture(id),
jni_facade_(jni_facade),
jni_facade_(std::move(jni_facade)),
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not quite sure why sometimes it's okay with value arguments like this, but at least it's still a net positive because it's finding move locations.

surface_texture_(surface_texture),
transform(SkMatrix::I()) {}

Expand Down
5 changes: 3 additions & 2 deletions shell/platform/android/android_image_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "flutter/shell/platform/android/android_image_generator.h"

#include <memory>
#include <utility>

#include <android/bitmap.h>
#include <android/hardware_buffer.h>
Expand All @@ -21,7 +22,7 @@ static jmethodID g_decode_image_method = nullptr;
AndroidImageGenerator::~AndroidImageGenerator() = default;

AndroidImageGenerator::AndroidImageGenerator(sk_sp<SkData> data)
: data_(data), image_info_(SkImageInfo::MakeUnknown(-1, -1)) {}
: data_(std::move(data)), image_info_(SkImageInfo::MakeUnknown(-1, -1)) {}

const SkImageInfo& AndroidImageGenerator::GetInfo() {
header_decoded_latch_.Wait();
Expand Down Expand Up @@ -173,7 +174,7 @@ bool AndroidImageGenerator::Register(JNIEnv* env) {

std::shared_ptr<ImageGenerator> AndroidImageGenerator::MakeFromData(
sk_sp<SkData> data,
fml::RefPtr<fml::TaskRunner> task_runner) {
const fml::RefPtr<fml::TaskRunner>& task_runner) {
std::shared_ptr<AndroidImageGenerator> generator(
new AndroidImageGenerator(std::move(data)));

Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/android_image_generator.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class AndroidImageGenerator : public ImageGenerator {

static std::shared_ptr<ImageGenerator> MakeFromData(
sk_sp<SkData> data,
fml::RefPtr<fml::TaskRunner> task_runner);
const fml::RefPtr<fml::TaskRunner>& task_runner);

static void NativeImageHeaderCallback(JNIEnv* env,
jclass jcaller,
Expand Down
15 changes: 7 additions & 8 deletions shell/platform/android/android_shell_holder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ static PlatformData GetDefaultPlatformData() {
}

AndroidShellHolder::AndroidShellHolder(
flutter::Settings settings,
const flutter::Settings& settings,
std::shared_ptr<PlatformViewAndroidJNI> jni_facade)
: settings_(std::move(settings)), jni_facade_(jni_facade) {
: settings_(settings), jni_facade_(jni_facade) {
static size_t thread_host_count = 1;
auto thread_label = std::to_string(thread_host_count++);

Expand Down Expand Up @@ -164,7 +164,7 @@ AndroidShellHolder::AndroidShellHolder(

shell_->RegisterImageDecoder(
[runner = task_runners.GetIOTaskRunner()](sk_sp<SkData> buffer) {
return AndroidImageGenerator::MakeFromData(buffer, runner);
return AndroidImageGenerator::MakeFromData(std::move(buffer), runner);
},
-1);
FML_DLOG(INFO) << "Registered Android SDK image decoder (API level 28+)";
Expand All @@ -181,7 +181,7 @@ AndroidShellHolder::AndroidShellHolder(
const std::shared_ptr<ThreadHost>& thread_host,
std::unique_ptr<Shell> shell,
const fml::WeakPtr<PlatformViewAndroid>& platform_view)
: settings_(std::move(settings)),
: settings_(settings),
jni_facade_(jni_facade),
platform_view_(platform_view),
thread_host_(thread_host),
Expand Down Expand Up @@ -335,13 +335,12 @@ std::optional<RunConfiguration> AndroidShellHolder::BuildRunConfiguration(

{
if (!entrypoint.empty() && !libraryUrl.empty()) {
config.SetEntrypointAndLibrary(std::move(entrypoint),
std::move(libraryUrl));
config.SetEntrypointAndLibrary(entrypoint, libraryUrl);
} else if (!entrypoint.empty()) {
config.SetEntrypoint(std::move(entrypoint));
config.SetEntrypoint(entrypoint);
}
if (!entrypoint_args.empty()) {
config.SetEntrypointArgs(std::move(entrypoint_args));
config.SetEntrypointArgs(entrypoint_args);
}
}
return config;
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/android_shell_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ namespace flutter {
///
class AndroidShellHolder {
public:
AndroidShellHolder(flutter::Settings settings,
AndroidShellHolder(const flutter::Settings& settings,
std::shared_ptr<PlatformViewAndroidJNI> jni_facade);

~AndroidShellHolder();
Expand Down
Loading