From c5e9cff7ef1b4a09dfebbff371f3dfe0a342190a Mon Sep 17 00:00:00 2001 From: Andrei Shikov Date: Tue, 14 Dec 2021 10:25:27 -0800 Subject: [PATCH] Clang-tidy fabric/jni package Summary: Applies suggestions from clang-tidy. The automatic linter seems to be broken atm, so I used default config from Android Studio, filtering out meaningless suggestions. Changelog: [Internal] Reviewed By: cortinico, sammy-SC Differential Revision: D32994138 fbshipit-source-id: e8edf53f0cb8b0eda4ff8bb8bf8f5196827efd68 --- .../react/fabric/jni/AsyncEventBeat.h | 4 +- .../react/fabric/jni/AsyncEventBeatV2.cpp | 4 +- .../com/facebook/react/fabric/jni/Binding.cpp | 90 +++++++++---------- .../com/facebook/react/fabric/jni/Binding.h | 31 ++++--- .../react/fabric/jni/CoreComponentsRegistry.h | 2 +- .../react/fabric/jni/EventBeatManager.h | 3 +- .../react/fabric/jni/EventEmitterWrapper.cpp | 9 +- .../react/fabric/jni/EventEmitterWrapper.h | 5 +- .../fabric/jni/ReactNativeConfigHolder.h | 2 +- .../react/fabric/jni/StateWrapperImpl.h | 4 - .../react/fabric/jni/SurfaceHandlerBinding.h | 3 - 11 files changed, 77 insertions(+), 80 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h index ee02ffc31d8..9a32c46f299 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h @@ -25,8 +25,8 @@ class AsyncEventBeat final : public EventBeat, public EventBeatManagerObserver { jni::global_ref javaUIManager) : EventBeat(ownerBox), eventBeatManager_(eventBeatManager), - runtimeExecutor_(runtimeExecutor), - javaUIManager_(javaUIManager) { + runtimeExecutor_(std::move(runtimeExecutor)), + javaUIManager_(std::move(javaUIManager)) { eventBeatManager->addObserver(*this); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeatV2.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeatV2.cpp index 1d9b2dd27f2..cb138bae429 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeatV2.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeatV2.cpp @@ -20,8 +20,8 @@ AsyncEventBeatV2::AsyncEventBeatV2( jni::global_ref javaUIManager) : EventBeat(ownerBox), eventBeatManager_(eventBeatManager), - runtimeExecutor_(runtimeExecutor), - javaUIManager_(javaUIManager) { + runtimeExecutor_(std::move(runtimeExecutor)), + javaUIManager_(std::move(javaUIManager)) { eventBeatManager->addObserver(*this); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 9f68687b4ad..6fe77f17931 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -48,61 +48,61 @@ struct JMountItem : public JavaClass { } // namespace -CppMountItem CppMountItem::CreateMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::CreateMountItem(ShadowView const &shadowView) { return {CppMountItem::Type::Create, {}, {}, shadowView, -1}; } -CppMountItem CppMountItem::DeleteMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::DeleteMountItem(ShadowView const &shadowView) { return {CppMountItem::Type::Delete, {}, shadowView, {}, -1}; } CppMountItem CppMountItem::InsertMountItem( - ShadowView parentView, - ShadowView shadowView, + ShadowView const &parentView, + ShadowView const &shadowView, int index) { return {CppMountItem::Type::Insert, parentView, {}, shadowView, index}; } CppMountItem CppMountItem::RemoveMountItem( - ShadowView parentView, - ShadowView shadowView, + ShadowView const &parentView, + ShadowView const &shadowView, int index) { return {CppMountItem::Type::Remove, parentView, shadowView, {}, index}; } -CppMountItem CppMountItem::UpdatePropsMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::UpdatePropsMountItem(ShadowView const &shadowView) { return {CppMountItem::Type::UpdateProps, {}, {}, shadowView, -1}; } -CppMountItem CppMountItem::UpdateStateMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::UpdateStateMountItem(ShadowView const &shadowView) { return {CppMountItem::Type::UpdateState, {}, {}, shadowView, -1}; } -CppMountItem CppMountItem::UpdateLayoutMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::UpdateLayoutMountItem(ShadowView const &shadowView) { return {CppMountItem::Type::UpdateLayout, {}, {}, shadowView, -1}; } -CppMountItem CppMountItem::UpdateEventEmitterMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::UpdateEventEmitterMountItem( + ShadowView const &shadowView) { return {CppMountItem::Type::UpdateEventEmitter, {}, {}, shadowView, -1}; } -CppMountItem CppMountItem::UpdatePaddingMountItem(ShadowView shadowView) { +CppMountItem CppMountItem::UpdatePaddingMountItem( + ShadowView const &shadowView) { return {CppMountItem::Type::UpdatePadding, {}, {}, shadowView, -1}; } static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) { - if (mountItemType == CppMountItem::Type::Create) { - return 2; // tag, isLayoutable - } else if (mountItemType == CppMountItem::Type::Insert) { - return 3; // tag, parentTag, index - } else if (mountItemType == CppMountItem::Type::Remove) { - return 3; // tag, parentTag, index - } else if (mountItemType == CppMountItem::Type::Delete) { - return 1; // tag - } else if (mountItemType == CppMountItem::Type::UpdateProps) { - return 1; // tag - } else if (mountItemType == CppMountItem::Type::UpdateState) { - return 1; // tag - } else if (mountItemType == CppMountItem::Type::UpdatePadding) { - return 5; // tag, top, left, bottom, right - } else if (mountItemType == CppMountItem::Type::UpdateLayout) { - return 6; // tag, x, y, w, h, DisplayType - } else if (mountItemType == CppMountItem::Type::UpdateEventEmitter) { - return 1; // tag - } else { - return -1; + switch (mountItemType) { + case CppMountItem::Type::Create: + return 2; // tag, isLayoutable + case CppMountItem::Type::Insert: + case CppMountItem::Type::Remove: + return 3; // tag, parentTag, index + case CppMountItem::Type::Delete: + case CppMountItem::Type::UpdateProps: + case CppMountItem::Type::UpdateState: + case CppMountItem::Type::UpdateEventEmitter: + return 1; // tag + case CppMountItem::Type::UpdatePadding: + return 5; // tag, top, left, bottom, right + case CppMountItem::Type::UpdateLayout: + return 6; // tag, x, y, w, h, DisplayType + case CppMountItem::Undefined: + case CppMountItem::Multiple: + return -1; } } @@ -254,7 +254,7 @@ Binding::getInspectorDataForInstance( result["selectedIndex"] = data.selectedIndex; result["props"] = data.props; auto hierarchy = folly::dynamic::array(); - for (auto hierarchyItem : data.hierarchy) { + for (const auto &hierarchyItem : data.hierarchy) { hierarchy.push_back(hierarchyItem); } result["hierarchy"] = hierarchy; @@ -634,12 +634,12 @@ void Binding::uninstallFabricUIManager() { } inline local_ref castReadableMap( - local_ref nativeMap) { + local_ref const &nativeMap) { return make_local(reinterpret_cast(nativeMap.get())); } inline local_ref castReadableArray( - local_ref nativeArray) { + local_ref const &nativeArray) { return make_local( reinterpret_cast(nativeArray.get())); } @@ -649,7 +649,7 @@ local_ref getPlatformComponentName(const ShadowView &shadowView) { static std::string scrollViewComponentName = std::string("ScrollView"); local_ref componentName; - if (scrollViewComponentName.compare(shadowView.componentName) == 0) { + if (scrollViewComponentName == shadowView.componentName) { auto newViewProps = std::static_pointer_cast(shadowView.props); if (newViewProps->getProbablyMoreHorizontalThanVertical_DEPRECATED()) { @@ -956,7 +956,7 @@ void Binding::schedulerDidFinishTransaction( LOG(ERROR) << "Unexpected CppMountItem type"; } } - if (cppUpdatePropsMountItems.size() > 0) { + if (!cppUpdatePropsMountItems.empty()) { writeIntBufferTypePreamble( CppMountItem::Type::UpdateProps, cppUpdatePropsMountItems.size(), @@ -975,7 +975,7 @@ void Binding::schedulerDidFinishTransaction( (*objBufferArray)[objBufferPosition++] = newPropsReadableMap.get(); } } - if (cppUpdateStateMountItems.size() > 0) { + if (!cppUpdateStateMountItems.empty()) { writeIntBufferTypePreamble( CppMountItem::Type::UpdateState, cppUpdateStateMountItems.size(), @@ -1003,7 +1003,7 @@ void Binding::schedulerDidFinishTransaction( (javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr); } } - if (cppUpdatePaddingMountItems.size() > 0) { + if (!cppUpdatePaddingMountItems.empty()) { writeIntBufferTypePreamble( CppMountItem::Type::UpdatePadding, cppUpdatePaddingMountItems.size(), @@ -1030,7 +1030,7 @@ void Binding::schedulerDidFinishTransaction( intBufferPosition += 5; } } - if (cppUpdateLayoutMountItems.size() > 0) { + if (!cppUpdateLayoutMountItems.empty()) { writeIntBufferTypePreamble( CppMountItem::Type::UpdateLayout, cppUpdateLayoutMountItems.size(), @@ -1043,10 +1043,10 @@ void Binding::schedulerDidFinishTransaction( auto pointScaleFactor = layoutMetrics.pointScaleFactor; auto frame = layoutMetrics.frame; - int x = round(scale(frame.origin.x, pointScaleFactor)); - int y = round(scale(frame.origin.y, pointScaleFactor)); - int w = round(scale(frame.size.width, pointScaleFactor)); - int h = round(scale(frame.size.height, pointScaleFactor)); + int x = (int)round(scale(frame.origin.x, pointScaleFactor)); + int y = (int)round(scale(frame.origin.y, pointScaleFactor)); + int w = (int)round(scale(frame.size.width, pointScaleFactor)); + int h = (int)round(scale(frame.size.height, pointScaleFactor)); int displayType = toInt(mountItem.newChildShadowView.layoutMetrics.displayType); @@ -1060,7 +1060,7 @@ void Binding::schedulerDidFinishTransaction( intBufferPosition += 6; } } - if (cppUpdateEventEmitterMountItems.size() > 0) { + if (!cppUpdateEventEmitterMountItems.empty()) { writeIntBufferTypePreamble( CppMountItem::Type::UpdateEventEmitter, cppUpdateEventEmitterMountItems.size(), @@ -1094,7 +1094,7 @@ void Binding::schedulerDidFinishTransaction( // requires that the differ never produces "DELETE...CREATE" in that order for // the same tag. It's nice to be able to batch all similar operations together // for space efficiency. - if (cppDeleteMountItems.size() > 0) { + if (!cppDeleteMountItems.empty()) { writeIntBufferTypePreamble( CppMountItem::Type::Delete, cppDeleteMountItems.size(), diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h index d022af57a3f..3d8ef22fd82 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h @@ -31,17 +31,21 @@ class Instance; struct CppMountItem final { #pragma mark - Designated Initializers - static CppMountItem CreateMountItem(ShadowView shadowView); - static CppMountItem DeleteMountItem(ShadowView shadowView); - static CppMountItem - InsertMountItem(ShadowView parentView, ShadowView shadowView, int index); - static CppMountItem - RemoveMountItem(ShadowView parentView, ShadowView shadowView, int index); - static CppMountItem UpdatePropsMountItem(ShadowView shadowView); - static CppMountItem UpdateStateMountItem(ShadowView shadowView); - static CppMountItem UpdateLayoutMountItem(ShadowView shadowView); - static CppMountItem UpdateEventEmitterMountItem(ShadowView shadowView); - static CppMountItem UpdatePaddingMountItem(ShadowView shadowView); + static CppMountItem CreateMountItem(ShadowView const &shadowView); + static CppMountItem DeleteMountItem(ShadowView const &shadowView); + static CppMountItem InsertMountItem( + ShadowView const &parentView, + ShadowView const &shadowView, + int index); + static CppMountItem RemoveMountItem( + ShadowView const &parentView, + ShadowView const &shadowView, + int index); + static CppMountItem UpdatePropsMountItem(ShadowView const &shadowView); + static CppMountItem UpdateStateMountItem(ShadowView const &shadowView); + static CppMountItem UpdateLayoutMountItem(ShadowView const &shadowView); + static CppMountItem UpdateEventEmitterMountItem(ShadowView const &shadowView); + static CppMountItem UpdatePaddingMountItem(ShadowView const &shadowView); #pragma mark - Type @@ -178,9 +182,8 @@ class Binding : public jni::HybridClass, std::mutex javaUIManagerMutex_; // LayoutAnimations - virtual void onAnimationStarted() override; - virtual void onAllAnimationsComplete() override; - LayoutAnimationDriver *getAnimationDriver(); + void onAnimationStarted() override; + void onAllAnimationsComplete() override; std::shared_ptr animationDriver_; std::unique_ptr backgroundExecutor_; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/CoreComponentsRegistry.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/CoreComponentsRegistry.h index 97d6cd3f180..dcf790a2b20 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/CoreComponentsRegistry.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/CoreComponentsRegistry.h @@ -23,7 +23,7 @@ class CoreComponentsRegistry static void registerNatives(); - CoreComponentsRegistry(ComponentFactory *delegate); + explicit CoreComponentsRegistry(ComponentFactory *delegate); static std::shared_ptr sharedProviderRegistry(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h index 094424ec260..9fc64e4e106 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h @@ -35,7 +35,8 @@ class EventBeatManager : public jni::HybridClass { static void registerNatives(); - EventBeatManager(jni::alias_ref jhybridobject); + explicit EventBeatManager( + jni::alias_ref jhybridobject); /* * Adds (or removes) observers. diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp index b535c2ca750..c2d53b5f62d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp @@ -19,12 +19,12 @@ EventEmitterWrapper::initHybrid(jni::alias_ref) { } void EventEmitterWrapper::invokeEvent( - std::string eventName, + std::string const &eventName, NativeMap *payload, int category) { if (eventEmitterPointer) { eventEmitterPointer->dispatchEvent( - std::move(eventName), + eventName, payload->consume(), EventPriority::AsynchronousBatched, static_cast(category)); @@ -44,12 +44,11 @@ void EventEmitterWrapper::invokeEvent( } void EventEmitterWrapper::invokeUniqueEvent( - std::string eventName, + std::string const &eventName, NativeMap *payload, int customCoalesceKey) { if (eventEmitterPointer) { - eventEmitterPointer->dispatchUniqueEvent( - std::move(eventName), payload->consume()); + eventEmitterPointer->dispatchUniqueEvent(eventName, payload->consume()); return; } // TODO: customCoalesceKey currently unused diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h index b36911da928..c1d1a71d993 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h @@ -26,9 +26,10 @@ class EventEmitterWrapper : public jni::HybridClass { SharedEventEmitter eventEmitter; EventEmitter const *eventEmitterPointer; - void invokeEvent(std::string eventName, NativeMap *params, int category); + void + invokeEvent(std::string const &eventName, NativeMap *params, int category); void invokeUniqueEvent( - std::string eventName, + std::string const &eventName, NativeMap *params, int customCoalesceKey); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/ReactNativeConfigHolder.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/ReactNativeConfigHolder.h index 4fe200a5ad4..761be149c7d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/ReactNativeConfigHolder.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/ReactNativeConfigHolder.h @@ -22,7 +22,7 @@ namespace react { */ class ReactNativeConfigHolder : public ReactNativeConfig { public: - ReactNativeConfigHolder(jni::alias_ref reactNativeConfig) + explicit ReactNativeConfigHolder(jni::alias_ref reactNativeConfig) : reactNativeConfig_(make_global(reactNativeConfig)){}; bool getBool(const std::string ¶m) const override; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h index 9193efc2518..866ad5b3b1f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h @@ -29,10 +29,6 @@ class StateWrapperImpl : public jni::HybridClass { jni::local_ref getStateMapBufferDataImpl(); jni::local_ref getStateDataImpl(); void updateStateImpl(NativeMap *map); - void updateStateWithFailureCallbackImpl( - NativeMap *map, - jni::alias_ref self, - int callbackRefId); State::Shared state_; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/SurfaceHandlerBinding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/SurfaceHandlerBinding.h index 27158eb2880..506fe088811 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/SurfaceHandlerBinding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/SurfaceHandlerBinding.h @@ -28,9 +28,6 @@ class SurfaceHandlerBinding : public jni::HybridClass { void setDisplayMode(jint mode); - void registerScheduler(std::shared_ptr scheduler); - void unregisterScheduler(std::shared_ptr scheduler); - jint getSurfaceId(); void setSurfaceId(jint surfaceId); jni::local_ref getModuleName();