Noissue - move work with a raw jni pointer to a separate function

libadblockplus-android/jni/JniFilterEngine.cpp

Index: libadblockplus-android/jni/JniFilterEngine.cpp
diff --git a/libadblockplus-android/jni/JniFilterEngine.cpp b/libadblockplus-android/jni/JniFilterEngine.cpp
index c70ec4cbb08aee084140909eef0b7c91427323ae..9001015bd057f904f2f8159778b927afdf64e37a 100644
--- a/libadblockplus-android/jni/JniFilterEngine.cpp
+++ b/libadblockplus-android/jni/JniFilterEngine.cpp
@@ -45,6 +45,14 @@ static AdblockPlus::FilterEngine::ContentType ConvertContentType(JNIEnv *env,
   return AdblockPlus::FilterEngine::StringToContentType(value);
 }
 
+namespace

[anton, 2017/06/15 05:16:53]

can we have the same code style like we use in
https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp...
like
`static AdblockPlus::FilterEnginePtr& GetFilterEnginePtrRef(jlong ptr) { ... }`

[sergei, 2017/06/16 10:44:36]

Yes we can but this function is related to JniFilterEngine (another codereview)
and I thought that it should be in the same namespace. Should it be static?

In general, "static" here is used merely for internal linkage, as far as I
understand and I'm not sure how it behaves when there is the same method
declaration in a header, for instance. I personally prefer to have all code in
proper namespaces, e.g. "abp" and anonymous, because they are more descriptive,
explicit and can have some other benefits. So, my opinion is to continue to use
static to stay consistent with the existing code when it makes sense but
consider switching from it.

+{
+  AdblockPlus::FilterEnginePtr& GetFilterEnginePtrRef(jlong ptr)
+  {
+    return *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  }
+}
+
 static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jlong jsEnginePtr, jlong isAllowedConnectionCallbackPtr)
 {
   try
@@ -95,7 +103,7 @@ static jboolean JNICALL JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
 {
   try
   {
-    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
     return engine->IsFirstRun() ? JNI_TRUE : JNI_FALSE;
   }
@@ -104,7 +112,7 @@ static jboolean JNICALL JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
 
 static jobject JNICALL JniGetFilter(JNIEnv* env, jclass clazz, jlong ptr, jstring jText)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   std::string text = JniJavaToStdString(env, jText);
 
   try
@@ -116,7 +124,7 @@ static jobject JNICALL JniGetFilter(JNIEnv* env, jclass clazz, jlong ptr, jstrin
 
 static jobject JNICALL JniGetListedFilters(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
@@ -136,7 +144,7 @@ static jobject JNICALL JniGetListedFilters(JNIEnv* env, jclass clazz, jlong ptr)
 
 static jobject JNICALL JniGetSubscription(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   std::string url = JniJavaToStdString(env, jUrl);
 
   try
@@ -148,7 +156,7 @@ static jobject JNICALL JniGetSubscription(JNIEnv* env, jclass clazz, jlong ptr,
 
 static void JNICALL JniShowNextNotification(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   std::string url = JniJavaToStdString(env, jUrl);
 
   try
@@ -161,8 +169,7 @@ static void JNICALL JniShowNextNotification(JNIEnv* env, jclass clazz, jlong ptr
 static void JNICALL JniSetShowNotificationCallback(JNIEnv* env, jclass clazz,
                                                   jlong ptr, jlong callbackPtr)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   JniShowNotificationCallback* const callback =
       JniLongToTypePtr<JniShowNotificationCallback>(callbackPtr);
@@ -181,7 +188,7 @@ static void JNICALL JniSetShowNotificationCallback(JNIEnv* env, jclass clazz,
 
 static void JNICALL JniRemoveShowNotificationCallback(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
@@ -192,7 +199,7 @@ static void JNICALL JniRemoveShowNotificationCallback(JNIEnv* env, jclass clazz,
 
 static jobject JNICALL JniGetListedSubscriptions(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
@@ -203,7 +210,7 @@ static jobject JNICALL JniGetListedSubscriptions(JNIEnv* env, jclass clazz, jlon
 
 static jobject JNICALL JniFetchAvailableSubscriptions(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
@@ -215,8 +222,7 @@ static jobject JNICALL JniFetchAvailableSubscriptions(JNIEnv* env, jclass clazz,
 static void JNICALL JniRemoveUpdateAvailableCallback(JNIEnv* env, jclass clazz,
                                                      jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   try
   {
     engine->RemoveUpdateAvailableCallback();
@@ -227,8 +233,7 @@ static void JNICALL JniRemoveUpdateAvailableCallback(JNIEnv* env, jclass clazz,
 static void JNICALL JniSetUpdateAvailableCallback(JNIEnv* env, jclass clazz,
                                                   jlong ptr, jlong callbackPtr)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   JniUpdateAvailableCallback* const callback =
       JniLongToTypePtr<JniUpdateAvailableCallback>(callbackPtr);
 
@@ -244,7 +249,7 @@ static void JNICALL JniSetUpdateAvailableCallback(JNIEnv* env, jclass clazz,
 
 static void JNICALL JniRemoveFilterChangeCallback(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
@@ -256,8 +261,7 @@ static void JNICALL JniRemoveFilterChangeCallback(JNIEnv* env, jclass clazz, jlo
 static void JNICALL JniSetFilterChangeCallback(JNIEnv* env, jclass clazz,
     jlong ptr, jlong filterPtr)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   JniFilterChangeCallback* callback = JniLongToTypePtr<JniFilterChangeCallback>(
       filterPtr);
 
@@ -275,7 +279,7 @@ static void JNICALL JniSetFilterChangeCallback(JNIEnv* env, jclass clazz,
 
 static void JNICALL JniForceUpdateCheck(JNIEnv* env, jclass clazz, jlong ptr, jlong updaterPtr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
   JniUpdateCheckDoneCallback* callback =
       JniLongToTypePtr<JniUpdateCheckDoneCallback>(updaterPtr);
 
@@ -299,8 +303,7 @@ static void JNICALL JniForceUpdateCheck(JNIEnv* env, jclass clazz, jlong ptr, jl
 static jobject JNICALL JniGetElementHidingSelectors(JNIEnv* env, jclass clazz,
     jlong ptr, jstring jDomain)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string domain = JniJavaToStdString(env, jDomain);
 
@@ -325,7 +328,7 @@ static jobject JNICALL JniGetElementHidingSelectors(JNIEnv* env, jclass clazz,
 
 static jobject JNICALL JniMatches(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl, jobject jContentType, jstring jDocumentUrl)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   AdblockPlus::FilterEngine::ContentType contentType =
@@ -363,7 +366,7 @@ static jobject JNICALL JniMatchesMany(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobject jContentType, jobjectArray jDocumentUrls)
 {
   AdblockPlus::FilterEnginePtr& engine =

[jens, 2017/07/05 09:27:53]

Minor one, but why don't have the definition of engine in only one line, like in
all other cases?

[anton, 2017/07/05 09:30:32]

yes, i believe previously the line was too long to fit into 100 characters.
now we can (like it's done in other places) and i'd say we should.

[sergei, 2017/07/05 13:46:20]

Right, I guess it's been simply overlooked when I was reverting other changes.

-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+      GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   AdblockPlus::FilterEngine::ContentType contentType =
@@ -384,8 +387,7 @@ static jobject JNICALL JniMatchesMany(JNIEnv* env, jclass clazz, jlong ptr,
 static jboolean JNICALL JniIsDocumentWhitelisted(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobjectArray jDocumentUrls)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   std::vector<std::string> documentUrls;
@@ -401,8 +403,7 @@ static jboolean JNICALL JniIsDocumentWhitelisted(JNIEnv* env, jclass clazz, jlon
 static jboolean JNICALL JniIsElemhideWhitelisted(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobjectArray jDocumentUrls)
 {
-  AdblockPlus::FilterEnginePtr& engine =
-      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   std::vector<std::string> documentUrls;
@@ -417,7 +418,7 @@ static jboolean JNICALL JniIsElemhideWhitelisted(JNIEnv* env, jclass clazz, jlon
 
 static jobject JNICALL JniGetPref(JNIEnv* env, jclass clazz, jlong ptr, jstring jPref)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string pref = JniJavaToStdString(env, jPref);
 
@@ -430,7 +431,7 @@ static jobject JNICALL JniGetPref(JNIEnv* env, jclass clazz, jlong ptr, jstring
 
 static void JNICALL JniSetPref(JNIEnv* env, jclass clazz, jlong ptr, jstring jPref, jlong jsValue)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string pref = JniJavaToStdString(env, jPref);
   const AdblockPlus::JsValue& value = JniGetJsValue(jsValue);
@@ -449,7 +450,7 @@ static jstring JNICALL JniGetHostFromURL(JNIEnv* env, jclass clazz, jlong ptr, j
     return NULL;
   }
 
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jurl);
   try
@@ -463,7 +464,7 @@ static jstring JNICALL JniGetHostFromURL(JNIEnv* env, jclass clazz, jlong ptr, j
 
 static void JNICALL JniSetAllowedConnectionType(JNIEnv* env, jclass clazz, jlong ptr, jstring jvalue)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string stdValue;
   const std::string* value = (jvalue != NULL
@@ -481,7 +482,7 @@ static jstring JNICALL JniGetAllowedConnectionType(JNIEnv* env, jclass clazz, jl
 {
   try
   {
-    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
     std::unique_ptr<std::string> value = engine->GetAllowedConnectionType();
 
     if (value == NULL)
@@ -496,7 +497,7 @@ static jstring JNICALL JniGetAllowedConnectionType(JNIEnv* env, jclass clazz, jl
 
 static void JNICALL JniSetAcceptableAdsEnabled(JNIEnv* env, jclass clazz, jlong ptr, jboolean jvalue)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
@@ -509,7 +510,7 @@ static jboolean JNICALL JniIsAcceptableAdsEnabled(JNIEnv* env, jclass clazz, jlo
 {
   try
   {
-    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
     return engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE;
   }
   CATCH_THROW_AND_RETURN(env, 0)
@@ -519,7 +520,7 @@ static jstring JNICALL JniGetAcceptableAdsSubscriptionURL(JNIEnv* env, jclass cl
 {
   try
   {
-    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
     std::string url = engine->GetAAUrl();
     return JniStdStringToJava(env, url);
   }