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

libadblockplus-android/jni/JniFilterEngine.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 27 matching lines @@
   JniLocalReference<jclass> contentTypeClass(env,
       env->GetObjectClass(jContentType));
   jmethodID nameMethod = env->GetMethodID(*contentTypeClass, "name",
       "()Ljava/lang/String;");
   JniLocalReference<jstring> jValue(env,
       (jstring) env->CallObjectMethod(jContentType, nameMethod));
   const std::string value = JniJavaToStdString(env, *jValue);
   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
   {
     AdblockPlus::JsEnginePtr& jsEngine = *JniLongToTypePtr<AdblockPlus::JsEnginePtr>(jsEnginePtr);
     AdblockPlus::FilterEnginePtr* filterEngine = NULL;
 
     if (isAllowedConnectionCallbackPtr != 0)
     {
       AdblockPlus::FilterEngine::CreationParameters creationParameters;
@@ skipping 30 matching lines @@
 
 static void JNICALL JniDtor(JNIEnv* env, jclass clazz, jlong ptr)
 {
   delete JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 }
 
 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;
   }
   CATCH_THROW_AND_RETURN(env, JNI_FALSE);
 }
 
 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
   {
     return NewJniFilter(env, engine->GetFilter(text));
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static jobject JNICALL JniGetListedFilters(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
     std::vector<AdblockPlus::Filter> filters = engine->GetListedFilters();
 
     jobject list = NewJniArrayList(env);
 
     for (std::vector<AdblockPlus::Filter>::iterator it = filters.begin(), end = filters.end(); it != end; it++)
     {
       JniAddObjectToList(env, list, *JniLocalReference<jobject>(env, NewJniFilter(env, std::move(*it))));
     }
 
     return list;
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 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
   {
     return NewJniSubscription(env, engine->GetSubscription(url));
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 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
   {
     engine->ShowNextNotification(url);
   }
   CATCH_AND_THROW(env);
 }
 
 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);
 
   auto showNotificationCallback = [callback](AdblockPlus::Notification&& notification)
   {
     callback->Callback(std::move(notification));
   };
 
   try
   {
     engine->SetShowNotificationCallback(showNotificationCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniRemoveShowNotificationCallback(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
     engine->RemoveShowNotificationCallback();
   }
   CATCH_AND_THROW(env);
 }
 
 static jobject JNICALL JniGetListedSubscriptions(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
     return SubscriptionsToArrayList(env, engine->GetListedSubscriptions());
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static jobject JNICALL JniFetchAvailableSubscriptions(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
     return SubscriptionsToArrayList(env, engine->FetchAvailableSubscriptions());
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 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();
   }
   CATCH_AND_THROW(env)
 }
 
 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);
 
   const AdblockPlus::FilterEngine::UpdateAvailableCallback updateAvailableCallback =
       std::bind(&JniUpdateAvailableCallback::Callback, callback,
                      std::placeholders::_1);
   try
   {
     engine->SetUpdateAvailableCallback(updateAvailableCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniRemoveFilterChangeCallback(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   try
   {
     engine->RemoveFilterChangeCallback();
   }
   CATCH_AND_THROW(env)
 }
 
 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);
 
   auto filterCallback = [callback](const std::string& arg, AdblockPlus::JsValue&& jsValue)
   {
     callback->Callback(arg, std::move(jsValue));
   };
 
   try
   {
     engine->SetFilterChangeCallback(filterCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 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);
 
   AdblockPlus::FilterEngine::UpdateCheckDoneCallback
       updateCheckDoneCallback = 0;
 
   if (updaterPtr)
   {
     updateCheckDoneCallback =
         std::bind(&JniUpdateCheckDoneCallback::Callback, callback,
                        std::placeholders::_1);
   }
 
   try
   {
     engine->ForceUpdateCheck(updateCheckDoneCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 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);
 
   try
   {
     std::vector<std::string> selectors = engine->GetElementHidingSelectors(
         domain);
 
     jobject list = NewJniArrayList(env);
 
     for (std::vector<std::string>::iterator it = selectors.begin(), end =
         selectors.end(); it != end; it++)
     {
       JniAddObjectToList(env, list,
           *JniLocalReference<jstring>(env, env->NewStringUTF(it->c_str())));
     }
 
     return list;
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 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 =
       ConvertContentType(env, jContentType);
   std::string documentUrl = JniJavaToStdString(env, jDocumentUrl);
 
   try
   {
     AdblockPlus::FilterPtr filterPtr = engine->Matches(url, contentType, documentUrl);
 
@@ skipping 16 matching lines @@
               *JniLocalReference<jstring>(env,
                   static_cast<jstring>(
                       env->GetObjectArrayElement(jArray, i)))));
     }
   }
 }
 
 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 =
       ConvertContentType(env, jContentType);
 
   std::vector<std::string> documentUrls;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
 
   try
   {
     AdblockPlus::FilterPtr filterPtr = engine->Matches(url, contentType, documentUrls);
 
     return (filterPtr.get() ? NewJniFilter(env, std::move(*filterPtr)) : 0);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 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;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
   try
   {
     return engine->IsDocumentWhitelisted(url, documentUrls) ?
         JNI_TRUE : JNI_FALSE;
   }
   CATCH_THROW_AND_RETURN(env, JNI_FALSE)
 }
 
 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;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
   try
   {
     return engine->IsElemhideWhitelisted(url, documentUrls) ?
         JNI_TRUE : JNI_FALSE;
   }
   CATCH_THROW_AND_RETURN(env, JNI_FALSE)
 }
 
 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);
 
   try
   {
     return NewJniJsValue(env, engine->GetPref(pref));
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 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);
 
   try
   {
     engine->SetPref(pref, value);
   }
   CATCH_AND_THROW(env)
 }
 
 static jstring JNICALL JniGetHostFromURL(JNIEnv* env, jclass clazz, jlong ptr, jstring jurl)
 {
   if (jurl == NULL)
   {
     return NULL;
   }
 
-  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jurl);
   try
   {
     std::string host = engine->GetHostFromURL(url);
 
     return JniStdStringToJava(env, host);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 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
     ? &(stdValue = JniJavaToStdString(env, jvalue))
     : NULL);
 
   try
   {
     engine->SetAllowedConnectionType(value);
   }
   CATCH_AND_THROW(env)
 }
 
 static jstring JNICALL JniGetAllowedConnectionType(JNIEnv* env, jclass clazz, jlong ptr)
 {
   try
   {
-    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
     std::unique_ptr<std::string> value = engine->GetAllowedConnectionType();
 
     if (value == NULL)
     {
       return NULL;
     }
 
     return JniStdStringToJava(env, *value.get());
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 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
   {
     engine->SetAAEnabled(jvalue == JNI_TRUE);
   }
   CATCH_AND_THROW(env)
 }
 
 static jboolean JNICALL JniIsAcceptableAdsEnabled(JNIEnv* env, jclass clazz, jlong ptr)
 {
   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)
 }
 
 static jstring JNICALL JniGetAcceptableAdsSubscriptionURL(JNIEnv* env, jclass clazz, jlong ptr)
 {
   try
   {
-    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
     std::string url = engine->GetAAUrl();
     return JniStdStringToJava(env, url);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static JNINativeMethod methods[] =
 {
   { (char*)"ctor", (char*)"(JJ)J", (void*)JniCtor },
   { (char*)"isFirstRun", (char*)"(J)Z", (void*)JniIsFirstRun },
@@ skipping 23 matching lines @@
   { (char*)"setAcceptableAdsEnabled", (char*)"(JZ)V", (void*)JniSetAcceptableAdsEnabled },
   { (char*)"isAcceptableAdsEnabled", (char*)"(J)Z", (void*)JniIsAcceptableAdsEnabled },
   { (char*)"getAcceptableAdsSubscriptionURL", (char*)"(J)Ljava/lang/String;", (void*)JniGetAcceptableAdsSubscriptionURL },
   { (char*)"dtor", (char*)"(J)V", (void*)JniDtor }
 };
 
 extern "C" JNIEXPORT void JNICALL Java_org_adblockplus_libadblockplus_FilterEngine_registerNatives(JNIEnv *env, jclass clazz)
 {
   env->RegisterNatives(clazz, methods, sizeof(methods) / sizeof(methods[0]));
 }