From e22606acd438c5c4b2725530dd884f0132cfab26 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Wed, 1 Nov 2023 13:56:38 -0700 Subject: [PATCH 1/5] draft objc smart pointers support arc fix license add unittests gn format format migrate VsyncWaiterIosTest.mm to arc try build.gn fix try build.gn fix exclude tests from license file fix license --- BUILD.gn | 1 + ci/licenses_golden/excluded_files | 2 + ci/licenses_golden/licenses_flutter | 4 + fml/BUILD.gn | 20 ++ fml/compiler_specific.h | 10 + fml/platform/darwin/scoped_block.h | 92 ++------ fml/platform/darwin/scoped_nsobject.h | 198 +++++++++++++----- fml/platform/darwin/scoped_nsobject.mm | 15 +- .../darwin/scoped_nsobject_unittests.mm | 93 ++++++++ .../darwin/scoped_nsobject_unittests_arc.mm | 131 ++++++++++++ fml/platform/darwin/scoped_policy.h | 25 +++ fml/platform/darwin/scoped_typeref.h | 137 ++++++++++++ shell/platform/darwin/ios/BUILD.gn | 2 +- .../framework/Source/FlutterPlatformViews.mm | 4 +- .../framework/Source/FlutterViewController.mm | 2 +- .../framework/Source/VsyncWaiterIosTest.mm | 24 +-- .../platform_message_response_darwin.mm | 2 +- .../ios/platform_message_handler_ios.mm | 3 +- testing/run_tests.py | 1 + 19 files changed, 619 insertions(+), 147 deletions(-) create mode 100644 fml/platform/darwin/scoped_nsobject_unittests.mm create mode 100644 fml/platform/darwin/scoped_nsobject_unittests_arc.mm create mode 100644 fml/platform/darwin/scoped_policy.h create mode 100644 fml/platform/darwin/scoped_typeref.h diff --git a/BUILD.gn b/BUILD.gn index 704c0f2c8628d..33ee6df14aabc 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -160,6 +160,7 @@ group("unittests") { "//flutter/display_list:display_list_unittests", "//flutter/flow:flow_unittests", "//flutter/fml:fml_unittests", + "//flutter/fml:fml_unittests_arc", "//flutter/lib/ui:ui_unittests", "//flutter/runtime:dart_plugin_registrant_unittests", "//flutter/runtime:no_dart_plugin_registrant_unittests", diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 211c6df6be377..2f4011aae75fb 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -101,6 +101,8 @@ ../../../flutter/fml/message_loop_unittests.cc ../../../flutter/fml/paths_unittests.cc ../../../flutter/fml/platform/darwin/cf_utils_unittests.mm +../../../flutter/fml/platform/darwin/scoped_nsobject_unittests.mm +../../../flutter/fml/platform/darwin/scoped_nsobject_unittests_arc.mm ../../../flutter/fml/platform/darwin/string_range_sanitization_unittests.mm ../../../flutter/fml/platform/win/file_win_unittests.cc ../../../flutter/fml/platform/win/wstring_conversion_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 0e2d2a6684c07..6acace41ac485 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -2795,6 +2795,8 @@ ORIGIN: ../../../flutter/fml/platform/darwin/scoped_nsautorelease_pool.cc + ../. ORIGIN: ../../../flutter/fml/platform/darwin/scoped_nsautorelease_pool.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/platform/darwin/scoped_nsobject.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/platform/darwin/scoped_nsobject.mm + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/fml/platform/darwin/scoped_policy.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/fml/platform/darwin/scoped_typeref.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/platform/darwin/string_range_sanitization.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/platform/darwin/string_range_sanitization.mm + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/platform/fuchsia/message_loop_fuchsia.cc + ../../../flutter/LICENSE @@ -5578,6 +5580,8 @@ FILE: ../../../flutter/fml/platform/darwin/scoped_nsautorelease_pool.cc FILE: ../../../flutter/fml/platform/darwin/scoped_nsautorelease_pool.h FILE: ../../../flutter/fml/platform/darwin/scoped_nsobject.h FILE: ../../../flutter/fml/platform/darwin/scoped_nsobject.mm +FILE: ../../../flutter/fml/platform/darwin/scoped_policy.h +FILE: ../../../flutter/fml/platform/darwin/scoped_typeref.h FILE: ../../../flutter/fml/platform/darwin/string_range_sanitization.h FILE: ../../../flutter/fml/platform/darwin/string_range_sanitization.mm FILE: ../../../flutter/fml/platform/fuchsia/message_loop_fuchsia.cc diff --git a/fml/BUILD.gn b/fml/BUILD.gn index 3faf7ceacbf78..c1cdd979c7699 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -165,6 +165,8 @@ source_set("fml") { "platform/darwin/scoped_nsautorelease_pool.h", "platform/darwin/scoped_nsobject.h", "platform/darwin/scoped_nsobject.mm", + "platform/darwin/scoped_policy.h", + "platform/darwin/scoped_typeref.h", "platform/darwin/string_range_sanitization.h", "platform/darwin/string_range_sanitization.mm", ] @@ -366,6 +368,10 @@ if (enable_unittests) { ] } + if (is_mac || is_ios) { + sources += [ "platform/darwin/scoped_nsobject_unittests.mm" ] + } + if (is_win) { sources += [ "platform/win/file_win_unittests.cc", @@ -388,4 +394,18 @@ if (enable_unittests) { [ "${fuchsia_sdk_path}/arch/${target_cpu}/sysroot/lib/libzircon.so" ] } } + + executable("fml_unittests_arc") { + testonly = true + if (is_mac || is_ios) { + cflags_objcc = flutter_cflags_objc_arc + sources = [ "platform/darwin/scoped_nsobject_unittests_arc.mm" ] + } + + deps = [ + ":fml_fixtures", + "//flutter/fml", + "//flutter/testing", + ] + } } diff --git a/fml/compiler_specific.h b/fml/compiler_specific.h index 52c9e6c9a4d9f..65f894496dae5 100644 --- a/fml/compiler_specific.h +++ b/fml/compiler_specific.h @@ -26,4 +26,14 @@ #define FML_ALLOW_UNUSED_TYPE #endif +// Annotate a function indicating the caller must examine the return value. +// Use like: +// int foo() WARN_UNUSED_RESULT; +#undef WARN_UNUSED_RESULT +#if defined(COMPILER_GCC) || defined(__clang__) +#define WARN_UNUSED_RESULT __attribute__((warn_unused_result)) +#else +#define WARN_UNUSED_RESULT +#endif + #endif // FLUTTER_FML_COMPILER_SPECIFIC_H_ diff --git a/fml/platform/darwin/scoped_block.h b/fml/platform/darwin/scoped_block.h index 319cb25d0d0ee..031266af0d065 100644 --- a/fml/platform/darwin/scoped_block.h +++ b/fml/platform/darwin/scoped_block.h @@ -8,86 +8,40 @@ #include #include "flutter/fml/compiler_specific.h" +#include "flutter/fml/platform/darwin/scoped_typeref.h" -namespace fml { - -// ScopedBlock<> is patterned after ScopedCFTypeRef<>, but uses Block_copy() and -// Block_release() instead of CFRetain() and CFRelease(). +#if defined(__has_feature) && __has_feature(objc_arc) +#define BASE_MAC_BRIDGE_CAST(TYPE, VALUE) (__bridge TYPE)(VALUE) +#else +#define BASE_MAC_BRIDGE_CAST(TYPE, VALUE) VALUE +#endif -enum class OwnershipPolicy { - // The scoped object takes ownership of an object by taking over an existing - // ownership claim. - kAssume, +namespace fml { - // The scoped object will retain the object and any initial ownership is - // not changed. - kRetain, -}; +namespace internal { template -class ScopedBlock { - public: - explicit ScopedBlock(B block = nullptr, - OwnershipPolicy policy = OwnershipPolicy::kAssume) - : block_(block) { - if (block_ && policy == OwnershipPolicy::kRetain) { - block_ = Block_copy(block); - } +struct ScopedBlockTraits { + static B InvalidValue() { return nullptr; } + static B Retain(B block) { + return BASE_MAC_BRIDGE_CAST( + B, Block_copy(BASE_MAC_BRIDGE_CAST(const void*, block))); } - - ScopedBlock(const ScopedBlock& that) : block_(that.block_) { - if (block_) { - block_ = Block_copy(block_); - } - } - - ~ScopedBlock() { - if (block_) { - Block_release(block_); - } - } - - ScopedBlock& operator=(const ScopedBlock& that) { - reset(that.get(), OwnershipPolicy::kRetain); - return *this; - } - - void reset(B block = nullptr, - OwnershipPolicy policy = OwnershipPolicy::kAssume) { - if (block && policy == OwnershipPolicy::kRetain) { - block = Block_copy(block); - } - if (block_) { - Block_release(block_); - } - block_ = block; + static void Release(B block) { + Block_release(BASE_MAC_BRIDGE_CAST(const void*, block)); } +}; - bool operator==(B that) const { return block_ == that; } - - bool operator!=(B that) const { return block_ != that; } - - // NOLINTNEXTLINE(google-explicit-constructor) - operator B() const { return block_; } - - B get() const { return block_; } - - void swap(ScopedBlock& that) { - B temp = that.block_; - that.block_ = block_; - block_ = temp; - } +} // namespace internal - [[nodiscard]] B release() { - B temp = block_; - block_ = nullptr; - return temp; - } +// ScopedBlock<> is patterned after ScopedCFTypeRef<>, but uses Block_copy() and +// Block_release() instead of CFRetain() and CFRelease(). - private: - B block_; -}; +template +using ScopedBlock = ScopedTypeRef>; } // namespace fml +#undef BASE_MAC_BRIDGE_CAST + #endif // FLUTTER_FML_PLATFORM_DARWIN_SCOPED_BLOCK_H_ diff --git a/fml/platform/darwin/scoped_nsobject.h b/fml/platform/darwin/scoped_nsobject.h index 9f910e44f31ed..d0acedea33466 100644 --- a/fml/platform/darwin/scoped_nsobject.h +++ b/fml/platform/darwin/scoped_nsobject.h @@ -5,6 +5,8 @@ #ifndef FLUTTER_FML_PLATFORM_DARWIN_SCOPED_NSOBJECT_H_ #define FLUTTER_FML_PLATFORM_DARWIN_SCOPED_NSOBJECT_H_ +#include + // Include NSObject.h directly because Foundation.h pulls in many dependencies. // (Approx 100k lines of code versus 1.5k for NSObject.h). scoped_nsobject gets // singled out because it is most typically included from other header files. @@ -12,8 +14,11 @@ #include "flutter/fml/compiler_specific.h" #include "flutter/fml/macros.h" +#include "flutter/fml/platform/darwin/scoped_typeref.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) @class NSAutoreleasePool; +#endif namespace fml { @@ -36,60 +41,91 @@ namespace fml { // scoped_nsautorelease_pool.h instead. // We check for bad uses of scoped_nsobject and NSAutoreleasePool at compile // time with a template specialization (see below). +// +// If Automatic Reference Counting (aka ARC) is enabled then the ownership +// policy is not controllable by the user as ARC make it really difficult to +// transfer ownership (the reference passed to scoped_nsobject constructor is +// sunk by ARC and __attribute((ns_consumed)) appears to not work correctly +// with Objective-C++ see https://llvm.org/bugs/show_bug.cgi?id=27887). Due to +// that, the policy is always to |RETAIN| when using ARC. + +namespace internal { + +id ScopedNSProtocolTraitsRetain(__unsafe_unretained id obj) + __attribute((ns_returns_not_retained)); +id ScopedNSProtocolTraitsAutoRelease(__unsafe_unretained id obj) + __attribute((ns_returns_not_retained)); +void ScopedNSProtocolTraitsRelease(__unsafe_unretained id obj); + +// Traits for ScopedTypeRef<>. As this class may be compiled from file with +// Automatic Reference Counting enable or not all methods have annotation to +// enforce the same code generation in both case (in particular, the Retain +// method uses ns_returns_not_retained to prevent ARC to insert a -release +// call on the returned value and thus defeating the -retain). +template +struct ScopedNSProtocolTraits { + static NST InvalidValue() __attribute((ns_returns_not_retained)) { + return nil; + } + static NST Retain(__unsafe_unretained NST nst) + __attribute((ns_returns_not_retained)) { + return ScopedNSProtocolTraitsRetain(nst); + } + static void Release(__unsafe_unretained NST nst) { + ScopedNSProtocolTraitsRelease(nst); + } +}; + +} // namespace internal template -class scoped_nsprotocol { +class scoped_nsprotocol + : public ScopedTypeRef> { public: - explicit scoped_nsprotocol(NST object = nil) : object_(object) {} + using Traits = internal::ScopedNSProtocolTraits; + +#if !defined(__has_feature) || !__has_feature(objc_arc) + explicit scoped_nsprotocol(NST object = Traits::InvalidValue(), + scoped_policy::OwnershipPolicy policy = + scoped_policy::OwnershipPolicy::kAssume) + : ScopedTypeRef(object, policy) {} +#else + explicit scoped_nsprotocol(NST object = Traits::InvalidValue()) + : ScopedTypeRef(object, + scoped_policy::OwnershipPolicy::kRetain) {} +#endif - scoped_nsprotocol(const scoped_nsprotocol& that) : object_([that.object_ retain]) {} + scoped_nsprotocol(const scoped_nsprotocol& that) + : ScopedTypeRef(that) {} - template - // NOLINTNEXTLINE(google-explicit-constructor) - scoped_nsprotocol(const scoped_nsprotocol& that) : object_([that.get() retain]) {} + template + explicit scoped_nsprotocol(const scoped_nsprotocol& that_as_subclass) + : ScopedTypeRef(that_as_subclass) {} - ~scoped_nsprotocol() { [object_ release]; } + scoped_nsprotocol(scoped_nsprotocol&& that) + : ScopedTypeRef(that) {} scoped_nsprotocol& operator=(const scoped_nsprotocol& that) { - reset([that.get() retain]); + ScopedTypeRef::operator=(that); return *this; } - void reset(NST object = nil) { - // We intentionally do not check that object != object_ as the caller must - // either already have an ownership claim over whatever it passes to this - // method, or call it with the |RETAIN| policy which will have ensured that - // the object is retained once more when reaching this point. - [object_ release]; - object_ = object; +#if !defined(__has_feature) || !__has_feature(objc_arc) + void reset(NST object = Traits::InvalidValue(), + scoped_policy::OwnershipPolicy policy = + scoped_policy::OwnershipPolicy::kAssume) { + ScopedTypeRef::reset(object, policy); } - - bool operator==(NST that) const { return object_ == that; } - bool operator!=(NST that) const { return object_ != that; } - - operator NST() const { return object_; } // NOLINT(google-explicit-constructor) - - NST get() const { return object_; } - - void swap(scoped_nsprotocol& that) { - NST temp = that.object_; - that.object_ = object_; - object_ = temp; +#else + void reset(NST object = Traits::InvalidValue()) { + ScopedTypeRef::reset(object, + scoped_policy::OwnershipPolicy::kRetain); } +#endif // Shift reference to the autorelease pool to be released later. - NST autorelease() { return [release() autorelease]; } - - private: - NST object_; - - // scoped_nsprotocol<>::release() is like scoped_ptr<>::release. It is NOT a - // wrapper for [object_ release]. To force a scoped_nsprotocol<> to call - // [object_ release], use scoped_nsprotocol<>::reset(). - [[nodiscard]] NST release() { - NST temp = object_; - object_ = nil; - return temp; + NST autorelease() __attribute((ns_returns_not_retained)) { + return internal::ScopedNSProtocolTraitsAutoRelease(this->release()); } }; @@ -112,46 +148,92 @@ bool operator!=(C p1, const scoped_nsprotocol& p2) { template class scoped_nsobject : public scoped_nsprotocol { public: - explicit scoped_nsobject(NST* object = nil) : scoped_nsprotocol(object) {} + using Traits = typename scoped_nsprotocol::Traits; - scoped_nsobject(const scoped_nsobject& that) : scoped_nsprotocol(that) {} +#if !defined(__has_feature) || !__has_feature(objc_arc) + explicit scoped_nsobject(NST* object = Traits::InvalidValue(), + scoped_policy::OwnershipPolicy policy = + scoped_policy::OwnershipPolicy::kAssume) + : scoped_nsprotocol(object, policy) {} +#else + explicit scoped_nsobject(NST* object = Traits::InvalidValue()) + : scoped_nsprotocol(object) {} +#endif - template - // NOLINTNEXTLINE(google-explicit-constructor) - scoped_nsobject(const scoped_nsobject& that) : scoped_nsprotocol(that) {} + scoped_nsobject(const scoped_nsobject& that) + : scoped_nsprotocol(that) {} + + template + explicit scoped_nsobject(const scoped_nsobject& that_as_subclass) + : scoped_nsprotocol(that_as_subclass) {} + + scoped_nsobject(scoped_nsobject&& that) + : scoped_nsprotocol(that) {} scoped_nsobject& operator=(const scoped_nsobject& that) { scoped_nsprotocol::operator=(that); return *this; } + +#if !defined(__has_feature) || !__has_feature(objc_arc) + void reset(NST* object = Traits::InvalidValue(), + scoped_policy::OwnershipPolicy policy = + scoped_policy::OwnershipPolicy::kAssume) { + scoped_nsprotocol::reset(object, policy); + } +#else + void reset(NST* object = Traits::InvalidValue()) { + scoped_nsprotocol::reset(object); + } +#endif + +#if !defined(__has_feature) || !__has_feature(objc_arc) + static_assert(std::is_same::value == false, + "Use ScopedNSAutoreleasePool instead"); +#endif }; // Specialization to make scoped_nsobject work. template <> class scoped_nsobject : public scoped_nsprotocol { public: - explicit scoped_nsobject(id object = nil) : scoped_nsprotocol(object) {} + using Traits = typename scoped_nsprotocol::Traits; - scoped_nsobject(const scoped_nsobject& that) : scoped_nsprotocol(that) {} +#if !defined(__has_feature) || !__has_feature(objc_arc) + explicit scoped_nsobject(id object = Traits::InvalidValue(), + scoped_policy::OwnershipPolicy policy = + scoped_policy::OwnershipPolicy::kAssume) + : scoped_nsprotocol(object, policy) {} +#else + explicit scoped_nsobject(id object = Traits::InvalidValue()) + : scoped_nsprotocol(object) {} +#endif - template - // NOLINTNEXTLINE(google-explicit-constructor) - scoped_nsobject(const scoped_nsobject& that) : scoped_nsprotocol(that) {} + scoped_nsobject(const scoped_nsobject& that) + : scoped_nsprotocol(that) {} + + template + explicit scoped_nsobject(const scoped_nsobject& that_as_subclass) + : scoped_nsprotocol(that_as_subclass) {} + + scoped_nsobject(scoped_nsobject&& that) : scoped_nsprotocol(that) {} scoped_nsobject& operator=(const scoped_nsobject& that) { scoped_nsprotocol::operator=(that); return *this; } -}; -// Do not use scoped_nsobject for NSAutoreleasePools, use -// ScopedNSAutoreleasePool instead. This is a compile time check. See details -// at top of header. -template <> -class scoped_nsobject { - private: - explicit scoped_nsobject(NSAutoreleasePool* object = nil); - FML_DISALLOW_COPY_AND_ASSIGN(scoped_nsobject); +#if !defined(__has_feature) || !__has_feature(objc_arc) + void reset(id object = Traits::InvalidValue(), + scoped_policy::OwnershipPolicy policy = + scoped_policy::OwnershipPolicy::kAssume) { + scoped_nsprotocol::reset(object, policy); + } +#else + void reset(id object = Traits::InvalidValue()) { + scoped_nsprotocol::reset(object); + } +#endif }; } // namespace fml diff --git a/fml/platform/darwin/scoped_nsobject.mm b/fml/platform/darwin/scoped_nsobject.mm index 8ff8d2934f57d..0645420b32df4 100644 --- a/fml/platform/darwin/scoped_nsobject.mm +++ b/fml/platform/darwin/scoped_nsobject.mm @@ -6,6 +6,19 @@ namespace fml { -// +namespace internal { +id ScopedNSProtocolTraitsRetain(id obj) { + return [obj retain]; +} + +id ScopedNSProtocolTraitsAutoRelease(id obj) { + return [obj autorelease]; +} + +void ScopedNSProtocolTraitsRelease(id obj) { + return [obj release]; +} + +} // namespace internal } // namespace fml diff --git a/fml/platform/darwin/scoped_nsobject_unittests.mm b/fml/platform/darwin/scoped_nsobject_unittests.mm new file mode 100644 index 0000000000000..b056969f4d72c --- /dev/null +++ b/fml/platform/darwin/scoped_nsobject_unittests.mm @@ -0,0 +1,93 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "flutter/fml/platform/darwin/scoped_nsautorelease_pool.h" +#include "flutter/fml/platform/darwin/scoped_nsobject.h" +#include "gtest/gtest.h" + +namespace { + +TEST(ScopedNSObjectTest, ScopedNSObject) { + fml::scoped_nsobject p1([[NSObject alloc] init]); + ASSERT_TRUE(p1.get()); + ASSERT_EQ(1u, [p1 retainCount]); + fml::scoped_nsobject p2(p1); + ASSERT_EQ(p1.get(), p2.get()); + ASSERT_EQ(2u, [p1 retainCount]); + p2.reset(); + ASSERT_EQ(nil, p2.get()); + ASSERT_EQ(1u, [p1 retainCount]); + { + fml::scoped_nsobject p3 = p1; + ASSERT_EQ(p1.get(), p3.get()); + ASSERT_EQ(2u, [p1 retainCount]); + p3 = p1; + ASSERT_EQ(p1.get(), p3.get()); + ASSERT_EQ(2u, [p1 retainCount]); + } + ASSERT_EQ(1u, [p1 retainCount]); + fml::scoped_nsobject p4([p1.get() retain]); + ASSERT_EQ(2u, [p1 retainCount]); + ASSERT_TRUE(p1 == p1.get()); + ASSERT_TRUE(p1 == p1); + ASSERT_FALSE(p1 != p1); + ASSERT_FALSE(p1 != p1.get()); + fml::scoped_nsobject p5([[NSObject alloc] init]); + ASSERT_TRUE(p1 != p5); + ASSERT_TRUE(p1 != p5.get()); + ASSERT_FALSE(p1 == p5); + ASSERT_FALSE(p1 == p5.get()); + + fml::scoped_nsobject p6 = p1; + ASSERT_EQ(3u, [p6 retainCount]); + { + fml::ScopedNSAutoreleasePool pool; + p6.autorelease(); + ASSERT_EQ(nil, p6.get()); + ASSERT_EQ(3u, [p1 retainCount]); + } + ASSERT_EQ(2u, [p1 retainCount]); +} + +// Instantiating scoped_nsobject<> with T=NSAutoreleasePool should trip a +// static_assert. +#if 0 +TEST(ScopedNSObjectTest, FailToCreateScopedNSObjectAutoreleasePool) { + fml::scoped_nsobject pool; +} +#endif + +TEST(ScopedNSObjectTest, ScopedNSObjectInContainer) { + fml::scoped_nsobject p([[NSObject alloc] init]); + ASSERT_TRUE(p.get()); + ASSERT_EQ(1u, [p retainCount]); + { + std::vector> objects; + objects.push_back(p); + ASSERT_EQ(2u, [p retainCount]); + ASSERT_EQ(p.get(), objects[0].get()); + objects.push_back(fml::scoped_nsobject([[NSObject alloc] init])); + ASSERT_TRUE(objects[1].get()); + ASSERT_EQ(1u, [objects[1] retainCount]); + } + ASSERT_EQ(1u, [p retainCount]); +} + +TEST(ScopedNSObjectTest, ScopedNSObjectFreeFunctions) { + fml::scoped_nsobject p1([[NSObject alloc] init]); + id o1 = p1.get(); + ASSERT_TRUE(o1 == p1); + ASSERT_FALSE(o1 != p1); + fml::scoped_nsobject p2([[NSObject alloc] init]); + ASSERT_TRUE(o1 != p2); + ASSERT_FALSE(o1 == p2); + id o2 = p2.get(); + swap(p1, p2); + ASSERT_EQ(o2, p1.get()); + ASSERT_EQ(o1, p2.get()); +} + +} // namespace diff --git a/fml/platform/darwin/scoped_nsobject_unittests_arc.mm b/fml/platform/darwin/scoped_nsobject_unittests_arc.mm new file mode 100644 index 0000000000000..cde666a00f354 --- /dev/null +++ b/fml/platform/darwin/scoped_nsobject_unittests_arc.mm @@ -0,0 +1,131 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#import + +#include "flutter/fml/platform/darwin/scoped_nsobject.h" +#include "fml/logging.h" +#include "gtest/gtest.h" + +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + +namespace { + +template +CFIndex GetRetainCount(const fml::scoped_nsobject& nst) { + @autoreleasepool { + return CFGetRetainCount((__bridge CFTypeRef)nst.get()) - 1; + } +} + +#if __has_feature(objc_arc_weak) +TEST(ScopedNSObjectTestARC, DefaultPolicyIsRetain) { + __weak id o; + @autoreleasepool { + fml::scoped_nsprotocol p([[NSObject alloc] init]); + o = p.get(); + EXPECT_EQ(o, p.get()); + } + EXPECT_EQ(o, nil); +} +#endif + +TEST(ScopedNSObjectTestARC, ScopedNSObject) { + fml::scoped_nsobject p1([[NSObject alloc] init]); + @autoreleasepool { + EXPECT_TRUE(p1.get()); + EXPECT_TRUE(p1.get()); + } + EXPECT_EQ(1, GetRetainCount(p1)); + EXPECT_EQ(1, GetRetainCount(p1)); + fml::scoped_nsobject p2(p1); + @autoreleasepool { + EXPECT_EQ(p1.get(), p2.get()); + } + EXPECT_EQ(2, GetRetainCount(p1)); + p2.reset(); + EXPECT_EQ(nil, p2.get()); + EXPECT_EQ(1, GetRetainCount(p1)); + { + fml::scoped_nsobject p3 = p1; + @autoreleasepool { + EXPECT_EQ(p1.get(), p3.get()); + } + EXPECT_EQ(2, GetRetainCount(p1)); + p3 = p1; + @autoreleasepool { + EXPECT_EQ(p1.get(), p3.get()); + } + EXPECT_EQ(2, GetRetainCount(p1)); + } + EXPECT_EQ(1, GetRetainCount(p1)); + fml::scoped_nsobject p4; + @autoreleasepool { + p4 = fml::scoped_nsobject(p1.get()); + } + EXPECT_EQ(2, GetRetainCount(p1)); + @autoreleasepool { + EXPECT_TRUE(p1 == p1.get()); + EXPECT_TRUE(p1 == p1); + EXPECT_FALSE(p1 != p1); + EXPECT_FALSE(p1 != p1.get()); + } + fml::scoped_nsobject p5([[NSObject alloc] init]); + @autoreleasepool { + EXPECT_TRUE(p1 != p5); + EXPECT_TRUE(p1 != p5.get()); + EXPECT_FALSE(p1 == p5); + EXPECT_FALSE(p1 == p5.get()); + } + + fml::scoped_nsobject p6 = p1; + EXPECT_EQ(3, GetRetainCount(p6)); + @autoreleasepool { + p6.autorelease(); + EXPECT_EQ(nil, p6.get()); + } + EXPECT_EQ(2, GetRetainCount(p1)); +} + +TEST(ScopedNSObjectTestARC, ScopedNSObjectInContainer) { + fml::scoped_nsobject p([[NSObject alloc] init]); + @autoreleasepool { + EXPECT_TRUE(p.get()); + } + EXPECT_EQ(1, GetRetainCount(p)); + @autoreleasepool { + std::vector> objects; + objects.push_back(p); + EXPECT_EQ(2, GetRetainCount(p)); + @autoreleasepool { + EXPECT_EQ(p.get(), objects[0].get()); + } + objects.push_back(fml::scoped_nsobject([[NSObject alloc] init])); + @autoreleasepool { + EXPECT_TRUE(objects[1].get()); + } + EXPECT_EQ(1, GetRetainCount(objects[1])); + } + EXPECT_EQ(1, GetRetainCount(p)); +} + +TEST(ScopedNSObjectTestARC, ScopedNSObjectFreeFunctions) { + fml::scoped_nsobject p1([[NSObject alloc] init]); + id o1 = p1.get(); + EXPECT_TRUE(o1 == p1); + EXPECT_FALSE(o1 != p1); + fml::scoped_nsobject p2([[NSObject alloc] init]); + EXPECT_TRUE(o1 != p2); + EXPECT_FALSE(o1 == p2); + id o2 = p2.get(); + swap(p1, p2); + EXPECT_EQ(o2, p1.get()); + EXPECT_EQ(o1, p2.get()); +} + +} // namespace diff --git a/fml/platform/darwin/scoped_policy.h b/fml/platform/darwin/scoped_policy.h new file mode 100644 index 0000000000000..fb4985871adae --- /dev/null +++ b/fml/platform/darwin/scoped_policy.h @@ -0,0 +1,25 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_FML_PLATFORM_DARWIN_SCOPED_POLICY_H_ +#define FLUTTER_FML_PLATFORM_DARWIN_SCOPED_POLICY_H_ + +namespace fml { +namespace scoped_policy { + +// Defines the ownership policy for a scoped object. +enum OwnershipPolicy { + // The scoped object takes ownership of an object by taking over an existing + // ownership claim. + kAssume, + + // The scoped object will retain the the object and any initial ownership is + // not changed. + kRetain +}; + +} // namespace scoped_policy +} // namespace fml + +#endif // FLUTTER_FML_PLATFORM_DARWIN_SCOPED_POLICY_H_ diff --git a/fml/platform/darwin/scoped_typeref.h b/fml/platform/darwin/scoped_typeref.h new file mode 100644 index 0000000000000..2dd55ca04ec5b --- /dev/null +++ b/fml/platform/darwin/scoped_typeref.h @@ -0,0 +1,137 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_FML_PLATFORM_DARWIN_SCOPED_TYPEREF_H_ +#define FLUTTER_FML_PLATFORM_DARWIN_SCOPED_TYPEREF_H_ + +#include "flutter/fml/compiler_specific.h" +#include "flutter/fml/platform/darwin/scoped_policy.h" + +namespace fml { + +// ScopedTypeRef<> is patterned after std::unique_ptr<>, but maintains ownership +// of a reference to any type that is maintained by Retain and Release methods. +// +// The Traits structure must provide the Retain and Release methods for type T. +// A default ScopedTypeRefTraits is used but not defined, and should be defined +// for each type to use this interface. For example, an appropriate definition +// of ScopedTypeRefTraits for CGLContextObj would be: +// +// template<> +// struct ScopedTypeRefTraits { +// static CGLContextObj InvalidValue() { return nullptr; } +// static CGLContextObj Retain(CGLContextObj object) { +// CGLContextRetain(object); +// return object; +// } +// static void Release(CGLContextObj object) { CGLContextRelease(object); } +// }; +// +// For the many types that have pass-by-pointer create functions, the function +// InitializeInto() is provided to allow direct initialization and assumption +// of ownership of the object. For example, continuing to use the above +// CGLContextObj specialization: +// +// fml::ScopedTypeRef context; +// CGLCreateContext(pixel_format, share_group, context.InitializeInto()); +// +// For initialization with an existing object, the caller may specify whether +// the ScopedTypeRef<> being initialized is assuming the caller's existing +// ownership of the object (and should not call Retain in initialization) or if +// it should not assume this ownership and must create its own (by calling +// Retain in initialization). This behavior is based on the |policy| parameter, +// with |kAssume| for the former and |kRetain| for the latter. The default +// policy is to |kAssume|. + +template +struct ScopedTypeRefTraits; + +template > +class ScopedTypeRef { + public: + typedef T element_type; + + explicit ScopedTypeRef( + __unsafe_unretained T object = Traits::InvalidValue(), + fml::scoped_policy::OwnershipPolicy policy = fml::scoped_policy::kAssume) + : object_(object) { + if (object_ && policy == fml::scoped_policy::kRetain) + object_ = Traits::Retain(object_); + } + + ScopedTypeRef(const ScopedTypeRef& that) : object_(that.object_) { + if (object_) + object_ = Traits::Retain(object_); + } + + // This allows passing an object to a function that takes its superclass. + template + explicit ScopedTypeRef(const ScopedTypeRef& that_as_subclass) + : object_(that_as_subclass.get()) { + if (object_) + object_ = Traits::Retain(object_); + } + + ScopedTypeRef(ScopedTypeRef&& that) : object_(that.object_) { + that.object_ = Traits::InvalidValue(); + } + + ~ScopedTypeRef() { + if (object_) + Traits::Release(object_); + } + + ScopedTypeRef& operator=(const ScopedTypeRef& that) { + reset(that.get(), fml::scoped_policy::kRetain); + return *this; + } + + // This is to be used only to take ownership of objects that are created + // by pass-by-pointer create functions. To enforce this, require that the + // object be reset to NULL before this may be used. + T* InitializeInto() WARN_UNUSED_RESULT { + DCHECK(!object_); + return &object_; + } + + void reset(__unsafe_unretained T object = Traits::InvalidValue(), + fml::scoped_policy::OwnershipPolicy policy = + fml::scoped_policy::kAssume) { + if (object && policy == fml::scoped_policy::kRetain) + object = Traits::Retain(object); + if (object_) + Traits::Release(object_); + object_ = object; + } + + bool operator==(__unsafe_unretained T that) const { return object_ == that; } + + bool operator!=(__unsafe_unretained T that) const { return object_ != that; } + + operator T() const __attribute((ns_returns_not_retained)) { return object_; } + + T get() const __attribute((ns_returns_not_retained)) { return object_; } + + void swap(ScopedTypeRef& that) { + __unsafe_unretained T temp = that.object_; + that.object_ = object_; + object_ = temp; + } + + // ScopedTypeRef<>::release() is like std::unique_ptr<>::release. It is NOT + // a wrapper for Release(). To force a ScopedTypeRef<> object to call + // Release(), use ScopedTypeRef<>::reset(). + T release() __attribute((ns_returns_not_retained)) WARN_UNUSED_RESULT { + __unsafe_unretained T temp = object_; + object_ = Traits::InvalidValue(); + return temp; + } + + private: + __unsafe_unretained T object_; +}; + +} // namespace fml + +#endif // FLUTTER_FML_PLATFORM_DARWIN_SCOPED_TYPEREF_H_ diff --git a/shell/platform/darwin/ios/BUILD.gn b/shell/platform/darwin/ios/BUILD.gn index cdb9e0d41e394..ed10ccee54625 100644 --- a/shell/platform/darwin/ios/BUILD.gn +++ b/shell/platform/darwin/ios/BUILD.gn @@ -243,7 +243,6 @@ source_set("ios_test_flutter_mrc") { "framework/Source/FlutterViewTest.mm", "framework/Source/SemanticsObjectTestMocks.h", "framework/Source/SemanticsObjectTest_mrc.mm", - "framework/Source/VsyncWaiterIosTest.mm", "framework/Source/accessibility_bridge_test.mm", "platform_message_handler_ios_test.mm", ] @@ -314,6 +313,7 @@ shared_library("ios_test_flutter") { "framework/Source/FlutterViewControllerTest.mm", "framework/Source/SemanticsObjectTest.mm", "framework/Source/UIViewController_FlutterScreenAndSceneIfLoadedTest.mm", + "framework/Source/VsyncWaiterIosTest.mm", "framework/Source/connection_collection_test.mm", ] deps = [ diff --git a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm index c0a7707283570..da76b2e107eaa 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm @@ -88,8 +88,8 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect, const std::shared_ptr& ios_context) { if (available_layer_index_ >= layers_.size()) { std::shared_ptr layer; - fml::scoped_nsobject overlay_view; - fml::scoped_nsobject overlay_view_wrapper; + fml::scoped_nsobject overlay_view; + fml::scoped_nsobject overlay_view_wrapper; if (!gr_context) { overlay_view.reset([[FlutterOverlayView alloc] init]); diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 7170bc1d4f4cd..9ebc028edf5ad 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -717,7 +717,7 @@ - (void)setSplashScreenView:(UIView*)view { } - (void)setFlutterViewDidRenderCallback:(void (^)(void))callback { - _flutterViewRenderedCallback.reset(callback, fml::OwnershipPolicy::kRetain); + _flutterViewRenderedCallback.reset(callback, fml::scoped_policy::OwnershipPolicy::kRetain); } #pragma mark - Surface creation and teardown updates diff --git a/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm b/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm index 39fc0341fc7bc..e8e254161b1ef 100644 --- a/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm +++ b/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm @@ -11,7 +11,7 @@ #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h" #import "flutter/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h" -FLUTTER_ASSERT_NOT_ARC +FLUTTER_ASSERT_ARC namespace { fml::RefPtr CreateNewThread(const std::string& name) { auto thread = std::make_unique(name); @@ -34,10 +34,9 @@ @implementation VsyncWaiterIosTest - (void)testSetAllowPauseAfterVsyncCorrect { auto thread_task_runner = CreateNewThread("VsyncWaiterIosTest"); - VSyncClient* vsyncClient = [[[VSyncClient alloc] + VSyncClient* vsyncClient = [[VSyncClient alloc] initWithTaskRunner:thread_task_runner - callback:[](std::unique_ptr recorder) {}] - autorelease]; + callback:[](std::unique_ptr recorder) {}]; CADisplayLink* link = [vsyncClient getDisplayLink]; vsyncClient.allowPauseAfterVsync = NO; [vsyncClient await]; @@ -60,8 +59,8 @@ - (void)testSetCorrectVariableRefreshRates { double maxFrameRate = 120; [[[mockDisplayLinkManager stub] andReturnValue:@(maxFrameRate)] displayRefreshRate]; - VSyncClient* vsyncClient = [[[VSyncClient alloc] initWithTaskRunner:thread_task_runner - callback:callback] autorelease]; + VSyncClient* vsyncClient = [[VSyncClient alloc] initWithTaskRunner:thread_task_runner + callback:callback]; CADisplayLink* link = [vsyncClient getDisplayLink]; if (@available(iOS 15.0, *)) { XCTAssertEqualWithAccuracy(link.preferredFrameRateRange.maximum, maxFrameRate, 0.1); @@ -82,8 +81,8 @@ - (void)testDoNotSetVariableRefreshRatesIfCADisableMinimumFrameDurationOnPhoneIs double maxFrameRate = 120; [[[mockDisplayLinkManager stub] andReturnValue:@(maxFrameRate)] displayRefreshRate]; - VSyncClient* vsyncClient = [[[VSyncClient alloc] initWithTaskRunner:thread_task_runner - callback:callback] autorelease]; + VSyncClient* vsyncClient = [[VSyncClient alloc] initWithTaskRunner:thread_task_runner + callback:callback]; CADisplayLink* link = [vsyncClient getDisplayLink]; if (@available(iOS 15.0, *)) { XCTAssertEqualWithAccuracy(link.preferredFrameRateRange.maximum, 0, 0.1); @@ -100,8 +99,8 @@ - (void)testDoNotSetVariableRefreshRatesIfCADisableMinimumFrameDurationOnPhoneIs id mockDisplayLinkManager = [OCMockObject mockForClass:[DisplayLinkManager class]]; double maxFrameRate = 120; [[[mockDisplayLinkManager stub] andReturnValue:@(maxFrameRate)] displayRefreshRate]; - VSyncClient* vsyncClient = [[[VSyncClient alloc] initWithTaskRunner:thread_task_runner - callback:callback] autorelease]; + VSyncClient* vsyncClient = [[VSyncClient alloc] initWithTaskRunner:thread_task_runner + callback:callback]; CADisplayLink* link = [vsyncClient getDisplayLink]; if (@available(iOS 15.0, *)) { XCTAssertEqualWithAccuracy(link.preferredFrameRateRange.maximum, 0, 0.1); @@ -114,10 +113,9 @@ - (void)testDoNotSetVariableRefreshRatesIfCADisableMinimumFrameDurationOnPhoneIs - (void)testAwaitAndPauseWillWorkCorrectly { auto thread_task_runner = CreateNewThread("VsyncWaiterIosTest"); - VSyncClient* vsyncClient = [[[VSyncClient alloc] + VSyncClient* vsyncClient = [[VSyncClient alloc] initWithTaskRunner:thread_task_runner - callback:[](std::unique_ptr recorder) {}] - autorelease]; + callback:[](std::unique_ptr recorder) {}]; CADisplayLink* link = [vsyncClient getDisplayLink]; XCTAssertTrue(link.isPaused); diff --git a/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm b/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm index ffe0f60802608..c823cf3f39bc2 100644 --- a/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm +++ b/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm @@ -9,7 +9,7 @@ PlatformMessageResponseDarwin::PlatformMessageResponseDarwin( PlatformMessageResponseCallback callback, fml::RefPtr platform_task_runner) - : callback_(callback, fml::OwnershipPolicy::kRetain), + : callback_(callback, fml::scoped_policy::OwnershipPolicy::kRetain), platform_task_runner_(std::move(platform_task_runner)) {} PlatformMessageResponseDarwin::~PlatformMessageResponseDarwin() = default; diff --git a/shell/platform/darwin/ios/platform_message_handler_ios.mm b/shell/platform/darwin/ios/platform_message_handler_ios.mm index ce585b6cb466c..38706459e00c3 100644 --- a/shell/platform/darwin/ios/platform_message_handler_ios.mm +++ b/shell/platform/darwin/ios/platform_message_handler_ios.mm @@ -130,7 +130,8 @@ - (void)dispatch:(dispatch_block_t)block { .task_queue = fml::scoped_nsprotocol( [static_cast*>(task_queue) retain]), .handler = - fml::ScopedBlock{handler, fml::OwnershipPolicy::kRetain}, + fml::ScopedBlock{ + handler, fml::scoped_policy::OwnershipPolicy::kRetain}, }; } } diff --git a/testing/run_tests.py b/testing/run_tests.py index 8074d81dd5b9f..1d4ade70f1031 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -403,6 +403,7 @@ def make_test(name, flags=None, extra_env=None): make_test('embedder_proctable_unittests'), make_test('embedder_unittests'), make_test('fml_unittests'), + make_test('fml_unittests_arc'), make_test('no_dart_plugin_registrant_unittests'), make_test('runtime_unittests'), make_test('testing_unittests'), From 68da8984a938219e4481dda222760182f871e356 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 3 Nov 2023 13:03:06 -0700 Subject: [PATCH 2/5] pick up change in https://codereview.chromium.org/2485463002/ --- fml/platform/darwin/scoped_nsobject.h | 7 ++++--- fml/platform/darwin/scoped_nsobject_unittests.mm | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/fml/platform/darwin/scoped_nsobject.h b/fml/platform/darwin/scoped_nsobject.h index d0acedea33466..ca1213c2c3677 100644 --- a/fml/platform/darwin/scoped_nsobject.h +++ b/fml/platform/darwin/scoped_nsobject.h @@ -103,7 +103,7 @@ class scoped_nsprotocol : ScopedTypeRef(that_as_subclass) {} scoped_nsprotocol(scoped_nsprotocol&& that) - : ScopedTypeRef(that) {} + : ScopedTypeRef(std::move(that)) {} scoped_nsprotocol& operator=(const scoped_nsprotocol& that) { ScopedTypeRef::operator=(that); @@ -168,7 +168,7 @@ class scoped_nsobject : public scoped_nsprotocol { : scoped_nsprotocol(that_as_subclass) {} scoped_nsobject(scoped_nsobject&& that) - : scoped_nsprotocol(that) {} + : scoped_nsprotocol(std::move(that)) {} scoped_nsobject& operator=(const scoped_nsobject& that) { scoped_nsprotocol::operator=(that); @@ -216,7 +216,8 @@ class scoped_nsobject : public scoped_nsprotocol { explicit scoped_nsobject(const scoped_nsobject& that_as_subclass) : scoped_nsprotocol(that_as_subclass) {} - scoped_nsobject(scoped_nsobject&& that) : scoped_nsprotocol(that) {} + scoped_nsobject(scoped_nsobject&& that) + : scoped_nsprotocol(std::move(that)) {} scoped_nsobject& operator=(const scoped_nsobject& that) { scoped_nsprotocol::operator=(that); diff --git a/fml/platform/darwin/scoped_nsobject_unittests.mm b/fml/platform/darwin/scoped_nsobject_unittests.mm index b056969f4d72c..4100b0af9a851 100644 --- a/fml/platform/darwin/scoped_nsobject_unittests.mm +++ b/fml/platform/darwin/scoped_nsobject_unittests.mm @@ -50,6 +50,12 @@ ASSERT_EQ(3u, [p1 retainCount]); } ASSERT_EQ(2u, [p1 retainCount]); + + fml::scoped_nsobject p7([NSObject new]); + fml::scoped_nsobject p8(std::move(p7)); + ASSERT_TRUE(p8); + ASSERT_EQ(1u, [p8 retainCount]); + ASSERT_FALSE(p7.get()); } // Instantiating scoped_nsobject<> with T=NSAutoreleasePool should trip a From cfccb751aff97555f1518aaa6b4758277d12ed74 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 3 Nov 2023 13:50:56 -0700 Subject: [PATCH 3/5] include utility fake init to supress warning --- fml/platform/darwin/scoped_nsobject.h | 1 + fml/platform/darwin/scoped_nsobject_unittests.mm | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/fml/platform/darwin/scoped_nsobject.h b/fml/platform/darwin/scoped_nsobject.h index ca1213c2c3677..82188c8de70b1 100644 --- a/fml/platform/darwin/scoped_nsobject.h +++ b/fml/platform/darwin/scoped_nsobject.h @@ -6,6 +6,7 @@ #define FLUTTER_FML_PLATFORM_DARWIN_SCOPED_NSOBJECT_H_ #include +#include // Include NSObject.h directly because Foundation.h pulls in many dependencies. // (Approx 100k lines of code versus 1.5k for NSObject.h). scoped_nsobject gets diff --git a/fml/platform/darwin/scoped_nsobject_unittests.mm b/fml/platform/darwin/scoped_nsobject_unittests.mm index 4100b0af9a851..566fcfd988923 100644 --- a/fml/platform/darwin/scoped_nsobject_unittests.mm +++ b/fml/platform/darwin/scoped_nsobject_unittests.mm @@ -10,6 +10,12 @@ namespace { +// This is to suppress the bugprone-use-after-move warning. +// This strategy is recommanded here: +// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html#silencing-erroneous-warnings +template +void IS_INITIALIZED(T&) {} + TEST(ScopedNSObjectTest, ScopedNSObject) { fml::scoped_nsobject p1([[NSObject alloc] init]); ASSERT_TRUE(p1.get()); @@ -51,10 +57,11 @@ } ASSERT_EQ(2u, [p1 retainCount]); - fml::scoped_nsobject p7([NSObject new]); + fml::scoped_nsobject p7([[NSObject alloc] init]); fml::scoped_nsobject p8(std::move(p7)); ASSERT_TRUE(p8); ASSERT_EQ(1u, [p8 retainCount]); + IS_INITIALIZED(p7); ASSERT_FALSE(p7.get()); } From 05a3a973d76223d3aa5e7fe5aa158aacc9cc5039 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Mon, 6 Nov 2023 12:41:16 -0800 Subject: [PATCH 4/5] adopt custom changes in flutter --- fml/platform/darwin/scoped_nsobject.h | 6 ++++++ fml/platform/darwin/scoped_typeref.h | 24 +++++++++++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/fml/platform/darwin/scoped_nsobject.h b/fml/platform/darwin/scoped_nsobject.h index 82188c8de70b1..86dd16481b587 100644 --- a/fml/platform/darwin/scoped_nsobject.h +++ b/fml/platform/darwin/scoped_nsobject.h @@ -96,6 +96,7 @@ class scoped_nsprotocol scoped_policy::OwnershipPolicy::kRetain) {} #endif + // NOLINTNEXTLINE(google-explicit-constructor) scoped_nsprotocol(const scoped_nsprotocol& that) : ScopedTypeRef(that) {} @@ -103,6 +104,7 @@ class scoped_nsprotocol explicit scoped_nsprotocol(const scoped_nsprotocol& that_as_subclass) : ScopedTypeRef(that_as_subclass) {} + // NOLINTNEXTLINE(google-explicit-constructor) scoped_nsprotocol(scoped_nsprotocol&& that) : ScopedTypeRef(std::move(that)) {} @@ -161,6 +163,7 @@ class scoped_nsobject : public scoped_nsprotocol { : scoped_nsprotocol(object) {} #endif + // NOLINTNEXTLINE(google-explicit-constructor) scoped_nsobject(const scoped_nsobject& that) : scoped_nsprotocol(that) {} @@ -168,6 +171,7 @@ class scoped_nsobject : public scoped_nsprotocol { explicit scoped_nsobject(const scoped_nsobject& that_as_subclass) : scoped_nsprotocol(that_as_subclass) {} + // NOLINTNEXTLINE(google-explicit-constructor) scoped_nsobject(scoped_nsobject&& that) : scoped_nsprotocol(std::move(that)) {} @@ -210,6 +214,7 @@ class scoped_nsobject : public scoped_nsprotocol { : scoped_nsprotocol(object) {} #endif + // NOLINTNEXTLINE(google-explicit-constructor) scoped_nsobject(const scoped_nsobject& that) : scoped_nsprotocol(that) {} @@ -217,6 +222,7 @@ class scoped_nsobject : public scoped_nsprotocol { explicit scoped_nsobject(const scoped_nsobject& that_as_subclass) : scoped_nsprotocol(that_as_subclass) {} + // NOLINTNEXTLINE(google-explicit-constructor) scoped_nsobject(scoped_nsobject&& that) : scoped_nsprotocol(std::move(that)) {} diff --git a/fml/platform/darwin/scoped_typeref.h b/fml/platform/darwin/scoped_typeref.h index 2dd55ca04ec5b..3d7faf144f408 100644 --- a/fml/platform/darwin/scoped_typeref.h +++ b/fml/platform/darwin/scoped_typeref.h @@ -56,30 +56,36 @@ class ScopedTypeRef { __unsafe_unretained T object = Traits::InvalidValue(), fml::scoped_policy::OwnershipPolicy policy = fml::scoped_policy::kAssume) : object_(object) { - if (object_ && policy == fml::scoped_policy::kRetain) + if (object_ && policy == fml::scoped_policy::kRetain) { object_ = Traits::Retain(object_); + } } + // NOLINTNEXTLINE(google-explicit-constructor) ScopedTypeRef(const ScopedTypeRef& that) : object_(that.object_) { - if (object_) + if (object_) { object_ = Traits::Retain(object_); + } } // This allows passing an object to a function that takes its superclass. template explicit ScopedTypeRef(const ScopedTypeRef& that_as_subclass) : object_(that_as_subclass.get()) { - if (object_) + if (object_) { object_ = Traits::Retain(object_); + } } + // NOLINTNEXTLINE(google-explicit-constructor) ScopedTypeRef(ScopedTypeRef&& that) : object_(that.object_) { that.object_ = Traits::InvalidValue(); } ~ScopedTypeRef() { - if (object_) + if (object_) { Traits::Release(object_); + } } ScopedTypeRef& operator=(const ScopedTypeRef& that) { @@ -98,10 +104,12 @@ class ScopedTypeRef { void reset(__unsafe_unretained T object = Traits::InvalidValue(), fml::scoped_policy::OwnershipPolicy policy = fml::scoped_policy::kAssume) { - if (object && policy == fml::scoped_policy::kRetain) + if (object && policy == fml::scoped_policy::kRetain) { object = Traits::Retain(object); - if (object_) + } + if (object_) { Traits::Release(object_); + } object_ = object; } @@ -109,6 +117,7 @@ class ScopedTypeRef { bool operator!=(__unsafe_unretained T that) const { return object_ != that; } + // NOLINTNEXTLINE(google-explicit-constructor) operator T() const __attribute((ns_returns_not_retained)) { return object_; } T get() const __attribute((ns_returns_not_retained)) { return object_; } @@ -119,10 +128,11 @@ class ScopedTypeRef { object_ = temp; } + protected: // ScopedTypeRef<>::release() is like std::unique_ptr<>::release. It is NOT // a wrapper for Release(). To force a ScopedTypeRef<> object to call // Release(), use ScopedTypeRef<>::reset(). - T release() __attribute((ns_returns_not_retained)) WARN_UNUSED_RESULT { + [[nodiscard]] T release() __attribute((ns_returns_not_retained)) { __unsafe_unretained T temp = object_; object_ = Traits::InvalidValue(); return temp; From d76831caa740f8cb806452426cee1543b327edf8 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 7 Nov 2023 09:37:15 -0800 Subject: [PATCH 5/5] review --- BUILD.gn | 2 +- ci/licenses_golden/excluded_files | 2 +- fml/BUILD.gn | 4 ++-- fml/compiler_specific.h | 10 ---------- ...ittests_arc.mm => scoped_nsobject_arc_unittests.mm} | 0 fml/platform/darwin/scoped_typeref.h | 4 ++-- testing/run_tests.py | 2 +- 7 files changed, 7 insertions(+), 17 deletions(-) rename fml/platform/darwin/{scoped_nsobject_unittests_arc.mm => scoped_nsobject_arc_unittests.mm} (100%) diff --git a/BUILD.gn b/BUILD.gn index 33ee6df14aabc..3a09a916af246 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -159,8 +159,8 @@ group("unittests") { "//flutter/display_list:display_list_rendertests", "//flutter/display_list:display_list_unittests", "//flutter/flow:flow_unittests", + "//flutter/fml:fml_arc_unittests", "//flutter/fml:fml_unittests", - "//flutter/fml:fml_unittests_arc", "//flutter/lib/ui:ui_unittests", "//flutter/runtime:dart_plugin_registrant_unittests", "//flutter/runtime:no_dart_plugin_registrant_unittests", diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 2f4011aae75fb..0ea98364aa258 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -101,8 +101,8 @@ ../../../flutter/fml/message_loop_unittests.cc ../../../flutter/fml/paths_unittests.cc ../../../flutter/fml/platform/darwin/cf_utils_unittests.mm +../../../flutter/fml/platform/darwin/scoped_nsobject_arc_unittests.mm ../../../flutter/fml/platform/darwin/scoped_nsobject_unittests.mm -../../../flutter/fml/platform/darwin/scoped_nsobject_unittests_arc.mm ../../../flutter/fml/platform/darwin/string_range_sanitization_unittests.mm ../../../flutter/fml/platform/win/file_win_unittests.cc ../../../flutter/fml/platform/win/wstring_conversion_unittests.cc diff --git a/fml/BUILD.gn b/fml/BUILD.gn index c1cdd979c7699..984b79ae82720 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -395,11 +395,11 @@ if (enable_unittests) { } } - executable("fml_unittests_arc") { + executable("fml_arc_unittests") { testonly = true if (is_mac || is_ios) { cflags_objcc = flutter_cflags_objc_arc - sources = [ "platform/darwin/scoped_nsobject_unittests_arc.mm" ] + sources = [ "platform/darwin/scoped_nsobject_arc_unittests.mm" ] } deps = [ diff --git a/fml/compiler_specific.h b/fml/compiler_specific.h index 65f894496dae5..52c9e6c9a4d9f 100644 --- a/fml/compiler_specific.h +++ b/fml/compiler_specific.h @@ -26,14 +26,4 @@ #define FML_ALLOW_UNUSED_TYPE #endif -// Annotate a function indicating the caller must examine the return value. -// Use like: -// int foo() WARN_UNUSED_RESULT; -#undef WARN_UNUSED_RESULT -#if defined(COMPILER_GCC) || defined(__clang__) -#define WARN_UNUSED_RESULT __attribute__((warn_unused_result)) -#else -#define WARN_UNUSED_RESULT -#endif - #endif // FLUTTER_FML_COMPILER_SPECIFIC_H_ diff --git a/fml/platform/darwin/scoped_nsobject_unittests_arc.mm b/fml/platform/darwin/scoped_nsobject_arc_unittests.mm similarity index 100% rename from fml/platform/darwin/scoped_nsobject_unittests_arc.mm rename to fml/platform/darwin/scoped_nsobject_arc_unittests.mm diff --git a/fml/platform/darwin/scoped_typeref.h b/fml/platform/darwin/scoped_typeref.h index 3d7faf144f408..e4728d43cd0bb 100644 --- a/fml/platform/darwin/scoped_typeref.h +++ b/fml/platform/darwin/scoped_typeref.h @@ -96,8 +96,8 @@ class ScopedTypeRef { // This is to be used only to take ownership of objects that are created // by pass-by-pointer create functions. To enforce this, require that the // object be reset to NULL before this may be used. - T* InitializeInto() WARN_UNUSED_RESULT { - DCHECK(!object_); + [[nodiscard]] T* InitializeInto() { + FML_DCHECK(!object_); return &object_; } diff --git a/testing/run_tests.py b/testing/run_tests.py index 1d4ade70f1031..8a139f9dfd365 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -403,7 +403,7 @@ def make_test(name, flags=None, extra_env=None): make_test('embedder_proctable_unittests'), make_test('embedder_unittests'), make_test('fml_unittests'), - make_test('fml_unittests_arc'), + make_test('fml_arc_unittests'), make_test('no_dart_plugin_registrant_unittests'), make_test('runtime_unittests'), make_test('testing_unittests'),