Turned "unnecessary value" and "move of const" lints to errors on mac (#36910)

gaaclarke · web-flow · commit f78a891d0f9c · 2022-10-22T00:38:54.000Z
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,4 +1,6 @@
 # Prefix check with "-" to ignore.
+# Note: Some of the checks here are used as errors selectively, see
+# //ci/lint.sh
 Checks: "bugprone-use-after-move,\
 clang-analyzer-*,\
 clang-diagnostic-*,\
@@ -19,6 +21,8 @@ performance-unnecessary-value-param"
 # Add checks when all warnings are fixed
 # to prevent new warnings being introduced.
 # https://github.com/flutter/flutter/issues/93279
+# Note: There are platform specific warnings as errors in
+# //ci/lint.sh
 WarningsAsErrors: "bugprone-use-after-move,\
 clang-analyzer-*,\
 readability-identifier-naming,\
diff --git a/ci/lint.sh b/ci/lint.sh
@@ -32,6 +32,9 @@ SRC_DIR="$(cd "$SCRIPT_DIR/../.."; pwd -P)"
 FLUTTER_DIR="$(cd "$SCRIPT_DIR/.."; pwd -P)"
 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"
 
 COMPILE_COMMANDS="$SRC_DIR/out/host_debug/compile_commands.json"
 if [ ! -f "$COMPILE_COMMANDS" ]; then
@@ -43,6 +46,7 @@ cd "$SCRIPT_DIR"
   --disable-dart-dev \
   "$SRC_DIR/flutter/tools/clang_tidy/bin/main.dart" \
   --src-dir="$SRC_DIR" \
+  --mac-host-warnings-as-errors="$MAC_HOST_WARNINGS_AS_ERRORS" \
   "$@"
 
 cd "$FLUTTER_DIR"
diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc
@@ -179,7 +179,7 @@ const std::optional<StencilAttachment>& RenderTarget::GetStencilAttachment()
 
 RenderTarget RenderTarget::CreateOffscreen(const Context& context,
                                            ISize size,
-                                           std::string label,
+                                           const std::string& label,
                                            StorageMode color_storage_mode,
                                            LoadAction color_load_action,
                                            StoreAction color_store_action,
@@ -230,7 +230,7 @@ RenderTarget RenderTarget::CreateOffscreen(const Context& context,
   stencil0.texture->SetLabel(SPrintF("%s Stencil Texture", label.c_str()));
 
   RenderTarget target;
-  target.SetColorAttachment(std::move(color0), 0u);
+  target.SetColorAttachment(color0, 0u);
   target.SetStencilAttachment(std::move(stencil0));
 
   return target;
@@ -239,7 +239,7 @@ RenderTarget RenderTarget::CreateOffscreen(const Context& context,
 RenderTarget RenderTarget::CreateOffscreenMSAA(
     const Context& context,
     ISize size,
-    std::string label,
+    const std::string& label,
     StorageMode color_storage_mode,
     StorageMode color_resolve_storage_mode,
     LoadAction color_load_action,
@@ -322,7 +322,7 @@ RenderTarget RenderTarget::CreateOffscreenMSAA(
   stencil0.texture->SetLabel(SPrintF("%s Stencil Texture", label.c_str()));
 
   RenderTarget target;
-  target.SetColorAttachment(std::move(color0), 0u);
+  target.SetColorAttachment(color0, 0u);
   target.SetStencilAttachment(std::move(stencil0));
 
   return target;
diff --git a/impeller/renderer/render_target.h b/impeller/renderer/render_target.h
@@ -22,7 +22,7 @@ class RenderTarget {
   static RenderTarget CreateOffscreen(
       const Context& context,
       ISize size,
-      std::string label = "Offscreen",
+      const std::string& label = "Offscreen",
       StorageMode color_storage_mode = StorageMode::kDevicePrivate,
       LoadAction color_load_action = LoadAction::kClear,
       StoreAction color_store_action = StoreAction::kStore,
@@ -33,7 +33,7 @@ class RenderTarget {
   static RenderTarget CreateOffscreenMSAA(
       const Context& context,
       ISize size,
-      std::string label = "Offscreen MSAA",
+      const std::string& label = "Offscreen MSAA",
       StorageMode color_storage_mode = StorageMode::kDeviceTransient,
       StorageMode color_resolve_storage_mode = StorageMode::kDevicePrivate,
       LoadAction color_load_action = LoadAction::kClear,
diff --git a/impeller/typographer/backends/skia/text_render_context_skia.cc b/impeller/typographer/backends/skia/text_render_context_skia.cc
@@ -47,10 +47,10 @@ static FontGlyphPair::Set CollectUniqueFontGlyphPairsSet(
 
 static FontGlyphPair::Vector CollectUniqueFontGlyphPairs(
     GlyphAtlas::Type type,
-    TextRenderContext::FrameIterator frame_iterator) {
+    const TextRenderContext::FrameIterator& frame_iterator) {
   TRACE_EVENT0("impeller", __FUNCTION__);
   FontGlyphPair::Vector vector;
-  auto set = CollectUniqueFontGlyphPairsSet(type, std::move(frame_iterator));
+  auto set = CollectUniqueFontGlyphPairsSet(type, frame_iterator);
   vector.reserve(set.size());
   for (const auto& item : set) {
     vector.emplace_back(item);
diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
@@ -80,15 +80,15 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) {
   LazyGlyphAtlas lazy_atlas;
   ASSERT_FALSE(lazy_atlas.HasColor());
 
-  lazy_atlas.AddTextFrame(std::move(frame));
+  lazy_atlas.AddTextFrame(frame);
 
   ASSERT_FALSE(lazy_atlas.HasColor());
 
   frame = TextFrameFromTextBlob(SkTextBlob::MakeFromString("😀 ", emoji_font));
 
   ASSERT_TRUE(frame.HasColor());
 
-  lazy_atlas.AddTextFrame(std::move(frame));
+  lazy_atlas.AddTextFrame(frame);
 
   ASSERT_TRUE(lazy_atlas.HasColor());
 }
diff --git a/lib/ui/compositing/scene.cc b/lib/ui/compositing/scene.cc
@@ -110,7 +110,7 @@ static sk_sp<DlImage> CreateDeferredImage(
       width, height, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
   return DlDeferredImageGPUSkia::MakeFromLayerTree(
       image_info, std::move(layer_tree), std::move(snapshot_delegate),
-      std::move(raster_task_runner), std::move(unref_queue));
+      raster_task_runner, std::move(unref_queue));
 }
 
 void Scene::RasterizeToImage(uint32_t width,
diff --git a/lib/ui/painting/image_encoding.cc b/lib/ui/painting/image_encoding.cc
@@ -275,9 +275,9 @@ Dart_Handle EncodeImage(CanvasImage* canvas_image,
        snapshot_delegate =
            UIDartState::Current()->GetSnapshotDelegate()]() mutable {
         EncodeImageAndInvokeDataCallback(
-            std::move(image), std::move(callback), image_format, ui_task_runner,
-            std::move(raster_task_runner), std::move(io_task_runner),
-            io_manager->GetResourceContext(), std::move(snapshot_delegate),
+            image, std::move(callback), image_format, ui_task_runner,
+            raster_task_runner, io_task_runner,
+            io_manager->GetResourceContext(), snapshot_delegate,
             io_manager->GetIsGpuDisabledSyncSwitch());
       }));
 
diff --git a/lib/ui/painting/immutable_buffer.cc b/lib/ui/painting/immutable_buffer.cc
@@ -139,8 +139,9 @@ Dart_Handle ImmutableBuffer::initFromFile(Dart_Handle raw_buffer_handle,
           sk_data = MakeSkDataWithCopy(bytes, buffer_size);
         }
         ui_task_runner->PostTask(
-            [sk_data = std::move(sk_data), ui_task = std::move(ui_task),
-             buffer_size]() { ui_task(sk_data, buffer_size); });
+            [sk_data = std::move(sk_data), ui_task = ui_task, buffer_size]() {
+              ui_task(sk_data, buffer_size);
+            });
       });
   return Dart_Null();
 }
diff --git a/lib/ui/painting/multi_frame_codec.cc b/lib/ui/painting/multi_frame_codec.cc
@@ -184,7 +184,7 @@ void MultiFrameCodec::State::GetNextFrameAndInvokeCallback(
   int duration = 0;
   sk_sp<DlImage> dlImage =
       GetNextFrameImage(std::move(resourceContext), gpu_disable_sync_switch,
-                        impeller_context, unref_queue);
+                        std::move(impeller_context), std::move(unref_queue));
   if (dlImage) {
     image = CanvasImage::Create();
     image->set_image(dlImage);
diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc
@@ -49,7 +49,7 @@ UIDartState::Context::Context(
       advisory_script_uri(std::move(advisory_script_uri)),
       advisory_script_entrypoint(std::move(advisory_script_entrypoint)),
       volatile_path_tracker(std::move(volatile_path_tracker)),
-      concurrent_task_runner(concurrent_task_runner),
+      concurrent_task_runner(std::move(concurrent_task_runner)),
       enable_impeller(enable_impeller) {}
 
 UIDartState::UIDartState(
diff --git a/shell/platform/embedder/tests/embedder_test_compositor.cc b/shell/platform/embedder/tests/embedder_test_compositor.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_compositor.h"
 
+#include <utility>
+
 #include "flutter/fml/logging.h"
 #include "flutter/shell/platform/embedder/tests/embedder_assertions.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -13,7 +15,7 @@ namespace testing {
 
 EmbedderTestCompositor::EmbedderTestCompositor(SkISize surface_size,
                                                sk_sp<GrDirectContext> context)
-    : surface_size_(surface_size), context_(context) {
+    : surface_size_(surface_size), context_(std::move(context)) {
   FML_CHECK(!surface_size_.isEmpty()) << "Surface size must not be empty";
 }
 
@@ -118,16 +120,17 @@ size_t EmbedderTestCompositor::GetBackingStoresCollectedCount() const {
 }
 
 void EmbedderTestCompositor::AddOnCreateRenderTargetCallback(
-    fml::closure callback) {
+    const fml::closure& callback) {
   on_create_render_target_callbacks_.push_back(callback);
 }
 
 void EmbedderTestCompositor::AddOnCollectRenderTargetCallback(
-    fml::closure callback) {
+    const fml::closure& callback) {
   on_collect_render_target_callbacks_.push_back(callback);
 }
 
-void EmbedderTestCompositor::AddOnPresentCallback(fml::closure callback) {
+void EmbedderTestCompositor::AddOnPresentCallback(
+    const fml::closure& callback) {
   on_present_callbacks_.push_back(callback);
 }
 
diff --git a/shell/platform/embedder/tests/embedder_test_compositor.h b/shell/platform/embedder/tests/embedder_test_compositor.h
@@ -64,11 +64,11 @@ class EmbedderTestCompositor {
 
   size_t GetBackingStoresCollectedCount() const;
 
-  void AddOnCreateRenderTargetCallback(fml::closure callback);
+  void AddOnCreateRenderTargetCallback(const fml::closure& callback);
 
-  void AddOnCollectRenderTargetCallback(fml::closure callback);
+  void AddOnCollectRenderTargetCallback(const fml::closure& callback);
 
-  void AddOnPresentCallback(fml::closure callback);
+  void AddOnPresentCallback(const fml::closure& callback);
 
   sk_sp<GrDirectContext> GetGrContext();
 
diff --git a/shell/platform/embedder/tests/embedder_test_compositor_gl.cc b/shell/platform/embedder/tests/embedder_test_compositor_gl.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_compositor_gl.h"
 
+#include <utility>
+
 #include "flutter/fml/logging.h"
 #include "flutter/shell/platform/embedder/tests/embedder_assertions.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -14,7 +16,7 @@ namespace testing {
 EmbedderTestCompositorGL::EmbedderTestCompositorGL(
     SkISize surface_size,
     sk_sp<GrDirectContext> context)
-    : EmbedderTestCompositor(surface_size, context) {}
+    : EmbedderTestCompositor(surface_size, std::move(context)) {}
 
 EmbedderTestCompositorGL::~EmbedderTestCompositorGL() = default;
 
diff --git a/shell/platform/embedder/tests/embedder_test_compositor_metal.cc b/shell/platform/embedder/tests/embedder_test_compositor_metal.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_compositor_metal.h"
 
+#include <utility>
+
 #include "flutter/fml/logging.h"
 #include "flutter/shell/platform/embedder/tests/embedder_assertions.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -14,7 +16,7 @@ namespace testing {
 EmbedderTestCompositorMetal::EmbedderTestCompositorMetal(
     SkISize surface_size,
     sk_sp<GrDirectContext> context)
-    : EmbedderTestCompositor(surface_size, context) {}
+    : EmbedderTestCompositor(surface_size, std::move(context)) {}
 
 EmbedderTestCompositorMetal::~EmbedderTestCompositorMetal() = default;
 
diff --git a/shell/platform/embedder/tests/embedder_test_compositor_vulkan.cc b/shell/platform/embedder/tests/embedder_test_compositor_vulkan.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_compositor_vulkan.h"
 
+#include <utility>
+
 #include "flutter/fml/logging.h"
 #include "flutter/shell/platform/embedder/tests/embedder_assertions.h"
 #include "flutter/shell/platform/embedder/tests/embedder_test_backingstore_producer.h"
@@ -15,7 +17,7 @@ namespace testing {
 EmbedderTestCompositorVulkan::EmbedderTestCompositorVulkan(
     SkISize surface_size,
     sk_sp<GrDirectContext> context)
-    : EmbedderTestCompositor(surface_size, context) {}
+    : EmbedderTestCompositor(surface_size, std::move(context)) {}
 
 EmbedderTestCompositorVulkan::~EmbedderTestCompositorVulkan() = default;
 
diff --git a/shell/platform/embedder/tests/embedder_test_context.cc b/shell/platform/embedder/tests/embedder_test_context.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_context.h"
 
+#include <utility>
+
 #include "flutter/fml/make_copyable.h"
 #include "flutter/fml/paths.h"
 #include "flutter/runtime/dart_vm.h"
@@ -94,7 +96,8 @@ void EmbedderTestContext::SetRootSurfaceTransformation(SkMatrix matrix) {
   root_surface_transformation_ = matrix;
 }
 
-void EmbedderTestContext::AddIsolateCreateCallback(fml::closure closure) {
+void EmbedderTestContext::AddIsolateCreateCallback(
+    const fml::closure& closure) {
   if (closure) {
     isolate_create_callbacks_.push_back(closure);
   }
@@ -226,7 +229,7 @@ void EmbedderTestContext::FireRootSurfacePresentCallbackIfPresent(
 
 void EmbedderTestContext::SetVsyncCallback(
     std::function<void(intptr_t)> callback) {
-  vsync_callback_ = callback;
+  vsync_callback_ = std::move(callback);
 }
 
 void EmbedderTestContext::RunVsyncCallback(intptr_t baton) {
diff --git a/shell/platform/embedder/tests/embedder_test_context.h b/shell/platform/embedder/tests/embedder_test_context.h
@@ -67,7 +67,7 @@ class EmbedderTestContext {
 
   void SetRootSurfaceTransformation(SkMatrix matrix);
 
-  void AddIsolateCreateCallback(fml::closure closure);
+  void AddIsolateCreateCallback(const fml::closure& closure);
 
   void AddNativeCallback(const char* name, Dart_NativeFunction function);
 
diff --git a/shell/platform/embedder/tests/embedder_test_context_gl.cc b/shell/platform/embedder/tests/embedder_test_context_gl.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_context_gl.h"
 
+#include <utility>
+
 #include "flutter/fml/make_copyable.h"
 #include "flutter/fml/paths.h"
 #include "flutter/runtime/dart_vm.h"
@@ -18,7 +20,7 @@ namespace flutter {
 namespace testing {
 
 EmbedderTestContextGL::EmbedderTestContextGL(std::string assets_path)
-    : EmbedderTestContext(assets_path) {}
+    : EmbedderTestContext(std::move(assets_path)) {}
 
 EmbedderTestContextGL::~EmbedderTestContextGL() {
   SetGLGetFBOCallback(nullptr);
@@ -61,18 +63,18 @@ bool EmbedderTestContextGL::GLPresent(FlutterPresentInfo present_info) {
 
 void EmbedderTestContextGL::SetGLGetFBOCallback(GLGetFBOCallback callback) {
   std::scoped_lock lock(gl_callback_mutex_);
-  gl_get_fbo_callback_ = callback;
+  gl_get_fbo_callback_ = std::move(callback);
 }
 
 void EmbedderTestContextGL::SetGLPopulateExistingDamageCallback(
     GLPopulateExistingDamageCallback callback) {
   std::scoped_lock lock(gl_callback_mutex_);
-  gl_populate_existing_damage_callback_ = callback;
+  gl_populate_existing_damage_callback_ = std::move(callback);
 }
 
 void EmbedderTestContextGL::SetGLPresentCallback(GLPresentCallback callback) {
   std::scoped_lock lock(gl_callback_mutex_);
-  gl_present_callback_ = callback;
+  gl_present_callback_ = std::move(callback);
 }
 
 uint32_t EmbedderTestContextGL::GLGetFramebuffer(FlutterFrameInfo frame_info) {
diff --git a/shell/platform/embedder/tests/embedder_test_context_software.cc b/shell/platform/embedder/tests/embedder_test_context_software.cc
@@ -4,6 +4,8 @@
 
 #include "flutter/shell/platform/embedder/tests/embedder_test_context_software.h"
 
+#include <utility>
+
 #include "flutter/fml/make_copyable.h"
 #include "flutter/fml/paths.h"
 #include "flutter/runtime/dart_vm.h"
@@ -18,11 +20,11 @@ namespace testing {
 
 EmbedderTestContextSoftware::EmbedderTestContextSoftware(
     std::string assets_path)
-    : EmbedderTestContext(assets_path) {}
+    : EmbedderTestContext(std::move(assets_path)) {}
 
 EmbedderTestContextSoftware::~EmbedderTestContextSoftware() = default;
 
-bool EmbedderTestContextSoftware::Present(sk_sp<SkImage> image) {
+bool EmbedderTestContextSoftware::Present(const sk_sp<SkImage>& image) {
   software_surface_present_count_++;
 
   FireRootSurfacePresentCallbackIfPresent([image] { return image; });
diff --git a/shell/platform/embedder/tests/embedder_test_context_software.h b/shell/platform/embedder/tests/embedder_test_context_software.h
@@ -21,7 +21,7 @@ class EmbedderTestContextSoftware : public EmbedderTestContext {
   // |EmbedderTestContext|
   EmbedderTestContextType GetContextType() const override;
 
-  bool Present(sk_sp<SkImage> image);
+  bool Present(const sk_sp<SkImage>& image);
 
  protected:
   virtual void SetupCompositor() override;
diff --git a/shell/platform/embedder/tests/embedder_unittests.cc b/shell/platform/embedder/tests/embedder_unittests.cc
diff --git a/shell/platform/embedder/tests/embedder_unittests_metal.mm b/shell/platform/embedder/tests/embedder_unittests_metal.mm
diff --git a/shell/platform/embedder/tests/embedder_unittests_util.cc b/shell/platform/embedder/tests/embedder_unittests_util.cc
diff --git a/shell/platform/embedder/tests/embedder_unittests_util.h b/shell/platform/embedder/tests/embedder_unittests_util.h
diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart
diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart
diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart
diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart

Original file line number	Diff line number	Diff line change
`@@ -110,7 +110,7 @@ static sk_sp<DlImage> CreateDeferredImage(`
`110`	`110`	`width, height, kRGBA_8888_SkColorType, kPremul_SkAlphaType);`
`111`	`111`	`return DlDeferredImageGPUSkia::MakeFromLayerTree(`
`112`	`112`	`image_info, std::move(layer_tree), std::move(snapshot_delegate),`
`113`		`- std::move(raster_task_runner), std::move(unref_queue));`
	`113`	`+ raster_task_runner, std::move(unref_queue));`
`114`	`114`	`}`
`115`	`115`
`116`	`116`	`void Scene::RasterizeToImage(uint32_t width,`
Original file line number	Diff line number	Diff line change
`@@ -139,8 +139,9 @@ Dart_Handle ImmutableBuffer::initFromFile(Dart_Handle raw_buffer_handle,`
`139`	`139`	`sk_data = MakeSkDataWithCopy(bytes, buffer_size);`
`140`	`140`	`}`
`141`	`141`	`ui_task_runner->PostTask(`
`142`		`- [sk_data = std::move(sk_data), ui_task = std::move(ui_task),`
`143`		`- buffer_size]() { ui_task(sk_data, buffer_size); });`
	`142`	`+ [sk_data = std::move(sk_data), ui_task = ui_task, buffer_size]() {`
	`143`	`+ ui_task(sk_data, buffer_size);`
	`144`	`+ });`
`144`	`145`	`});`
`145`	`146`	`return Dart_Null();`
`146`	`147`	`}`