mobile: API registration takes string&&, retrieval takes string_view (e…

…nvoyproxy#32056) `string&&` makes it clear on setting that the `name` param will be `std::move`'d when creating the key. `string_view` for lookup because flat_hash_map supports heterogenous lookups: https://abseil.io/docs/cpp/guides/container#heterogeneous-lookup Signed-off-by: Ali Beyad <[email protected]>
kyessenov · Jan 26, 2024 · dc69662 · dc69662
1 parent 5d2f3ae
commit dc69662
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 15 deletions.
diff --git a/mobile/library/common/api/external.cc b/mobile/library/common/api/external.cc
@@ -1,5 +1,7 @@
 #include "external.h"
 
+#include <string>
+
 #include "source/common/common/assert.h"
 
 #include "absl/container/flat_hash_map.h"
@@ -15,14 +17,14 @@ static absl::flat_hash_map<std::string, void*> registry_{};
 // TODO(goaway): To expose this for general usage, it will need to be made thread-safe. For now it
 // relies on the assumption that usage will occur only as part of Engine configuration, and thus be
 // limited to a single thread.
-void registerApi(std::string name, void* api) {
+void registerApi(std::string&& name, void* api) {
   RELEASE_ASSERT(api != nullptr, "cannot register null API");
-  registry_[name] = api;
+  registry_[std::move(name)] = api;
 }
 
 // TODO(goaway): This is not thread-safe, but the assumption here is that all writes will complete
 // before any reads occur.
-void* retrieveApi(std::string name, bool allow_absent) {
+void* retrieveApi(absl::string_view name, bool allow_absent) {
   void* api = registry_[name];
   if (!allow_absent) {
     RELEASE_ASSERT(api != nullptr, fmt::format("{} not registered", name));

diff --git a/mobile/library/common/api/external.h b/mobile/library/common/api/external.h
@@ -2,19 +2,21 @@
 
 #include <string>
 
+#include "absl/strings/string_view.h"
+
 namespace Envoy {
 namespace Api {
 namespace External {
 
 /**
  * Register an external runtime API for usage (e.g. in extensions).
  */
-void registerApi(std::string name, void* api);
+void registerApi(std::string&& name, void* api);
 
 /**
  * Retrieve an external runtime API for usage (e.g. in extensions).
  */
-void* retrieveApi(std::string name, bool allow_absent = false);
+void* retrieveApi(absl::string_view name, bool allow_absent = false);
 
 } // namespace External
 } // namespace Api

diff --git a/mobile/library/common/jni/jni_impl.cc b/mobile/library/common/jni/jni_impl.cc
@@ -964,8 +964,7 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_registerKeyValueStore(JNIEnv* e
 
   Envoy::JNI::JniHelper jni_helper(env);
   Envoy::JNI::StringUtfUniquePtr native_java_string = jni_helper.getStringUtfChars(name, nullptr);
-  std::string api_name(native_java_string.get());
-  Envoy::Api::External::registerApi(api_name, api);
+  Envoy::Api::External::registerApi(/*name=*/std::string(native_java_string.get()), api);
   return ENVOY_SUCCESS;
 }
 
@@ -1003,8 +1002,7 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_registerFilterFactory(JNIEnv* e
   Envoy::JNI::JniHelper jni_helper(env);
   Envoy::JNI::StringUtfUniquePtr native_java_string =
       jni_helper.getStringUtfChars(filter_name, nullptr);
-  std::string api_name(native_java_string.get());
-  Envoy::Api::External::registerApi(api_name, api);
+  Envoy::Api::External::registerApi(/*name=*/std::string(native_java_string.get()), api);
   return ENVOY_SUCCESS;
 }
 
@@ -1129,8 +1127,8 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_registerStringAccessor(JNIEnv*
   Envoy::JNI::JniHelper jni_helper(env);
   Envoy::JNI::StringUtfUniquePtr native_java_string =
       jni_helper.getStringUtfChars(accessor_name, nullptr);
-  std::string api_name(native_java_string.get());
-  Envoy::Api::External::registerApi(api_name, string_accessor);
+  Envoy::Api::External::registerApi(/*name=*/std::string(native_java_string.get()),
+                                    string_accessor);
   return ENVOY_SUCCESS;
 }
 

diff --git a/mobile/test/common/common/lambda_logger_delegate_test.cc b/mobile/test/common/common/lambda_logger_delegate_test.cc
@@ -16,7 +16,7 @@ class LambdaDelegateTest : public testing::Test {
   static envoy_event_tracker tracker;
 
   static void SetUpTestSuite() {
-    Api::External::registerApi(envoy_event_tracker_api_name, &tracker);
+    Api::External::registerApi(std::string(envoy_event_tracker_api_name), &tracker);
   }
 };
 

diff --git a/...common/extensions/filters/http/platform_bridge/platform_bridge_filter_integration_test.cc b/...common/extensions/filters/http/platform_bridge/platform_bridge_filter_integration_test.cc
@@ -33,8 +33,8 @@ class PlatformBridgeIntegrationTest : public testing::TestWithParam<Network::Add
     setDownstreamProtocol(Http::CodecClient::Type::HTTP2);
     setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
 
-    Api::External::registerApi("filter_1", filter_1);
-    Api::External::registerApi("filter_2", filter_2);
+    Api::External::registerApi(std::string("filter_1"), filter_1);
+    Api::External::registerApi(std::string("filter_2"), filter_2);
 
     config_helper_.addFilter(filter_config_1);
     config_helper_.addFilter(filter_config_2);

diff --git a/mobile/test/common/extensions/filters/http/platform_bridge/platform_bridge_filter_test.cc b/mobile/test/common/extensions/filters/http/platform_bridge/platform_bridge_filter_test.cc
@@ -43,7 +43,7 @@ class PlatformBridgeFilterTest : public testing::Test {
   void setUpFilter(std::string name, envoy_http_filter* platform_filter) {
     envoymobile::extensions::filters::http::platform_bridge::PlatformBridge config;
     config.set_platform_filter_name(name);
-    Api::External::registerApi(config.platform_filter_name(), platform_filter);
+    Api::External::registerApi(std::string(config.platform_filter_name()), platform_filter);
 
     config_ = std::make_shared<PlatformBridgeFilterConfig>(context_, config);
     filter_ = std::make_shared<PlatformBridgeFilter>(config_, dispatcher_);