From 17a08eeca072d101f9155a8dafb41bafe75ee58e Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Fri, 14 Jul 2023 01:32:50 +0000 Subject: [PATCH] aconfig: update c/c++ codegen Two major changes to c/c++ codegen (1) explicit setter for each flag instead of a generic flag setter void override_flag(std::string name, bool val) is replaced with void (bool val) for each flag name This has several advantages: (a) generated code is more c++ idomatic (b) no longer need to create flag name string constants (c) any typo in the code is caught early in the build time (2) remove flag setter and flag override reset methods/functions when generating code targets for production. If developers want to update their main function to take command line arg for flag overrides, they can use compile time macros to decide if the flag override code should be included. Bug: b/279483801 Test: atest aconfig.test Change-Id: I6141f7f979b32fe0426154d578edeb997ae5ff7c --- tools/aconfig/src/codegen_cpp.rs | 201 +++++++++++------- .../templates/c_exported_header.template | 17 +- .../aconfig/templates/c_source_file.template | 19 +- .../templates/cpp_exported_header.template | 36 ++-- .../templates/cpp_source_file.template | 2 - .../templates/cpp_test_flag_provider.template | 25 +-- 6 files changed, 174 insertions(+), 126 deletions(-) diff --git a/tools/aconfig/src/codegen_cpp.rs b/tools/aconfig/src/codegen_cpp.rs index 979b8ad893..ff9e8ef0bc 100644 --- a/tools/aconfig/src/codegen_cpp.rs +++ b/tools/aconfig/src/codegen_cpp.rs @@ -42,7 +42,7 @@ where cpp_namespace, package: package.to_string(), readwrite, - for_prod: codegen_mode == CodegenMode::Production, + for_test: codegen_mode == CodegenMode::Test, class_elements, }; @@ -102,7 +102,7 @@ pub struct Context { pub cpp_namespace: String, pub package: String, pub readwrite: bool, - pub for_prod: bool, + pub for_test: bool, pub class_elements: Vec, } @@ -111,7 +111,6 @@ pub struct ClassElement { pub readwrite: bool, pub default_value: String, pub flag_name: String, - pub uppercase_flag_name: String, pub device_config_namespace: String, pub device_config_flag: String, } @@ -125,7 +124,6 @@ fn create_class_element(package: &str, pf: &ProtoParsedFlag) -> ClassElement { "false".to_string() }, flag_name: pf.name().to_string(), - uppercase_flag_name: pf.name().to_string().to_ascii_uppercase(), device_config_namespace: pf.namespace().to_string(), device_config_flag: codegen::create_device_config_ident(package, pf.name()) .expect("values checked at flag parse time"), @@ -157,19 +155,10 @@ public: virtual bool enabled_ro() = 0; virtual bool enabled_rw() = 0; - - virtual void override_flag(std::string const&, bool) {} - - virtual void reset_overrides() {} }; extern std::unique_ptr provider_; -extern std::string const DISABLED_RO; -extern std::string const DISABLED_RW; -extern std::string const ENABLED_RO; -extern std::string const ENABLED_RW; - inline bool disabled_ro() { return false; } @@ -186,14 +175,6 @@ inline bool enabled_rw() { return provider_->enabled_rw(); } -inline void override_flag(std::string const& name, bool val) { - return provider_->override_flag(name, val); -} - -inline void reset_overrides() { - return provider_->reset_overrides(); -} - } #endif "#; @@ -213,46 +194,59 @@ public: virtual bool disabled_ro() = 0; + virtual void disabled_ro(bool val) = 0; + virtual bool disabled_rw() = 0; + virtual void disabled_rw(bool val) = 0; + virtual bool enabled_ro() = 0; + virtual void enabled_ro(bool val) = 0; + virtual bool enabled_rw() = 0; - virtual void override_flag(std::string const&, bool) {} + virtual void enabled_rw(bool val) = 0; - virtual void reset_overrides() {} + virtual void reset_flags() {} }; extern std::unique_ptr provider_; -extern std::string const DISABLED_RO; -extern std::string const DISABLED_RW; -extern std::string const ENABLED_RO; -extern std::string const ENABLED_RW; - inline bool disabled_ro() { return provider_->disabled_ro(); } +inline void disabled_ro(bool val) { + provider_->disabled_ro(val); +} + inline bool disabled_rw() { return provider_->disabled_rw(); } +inline void disabled_rw(bool val) { + provider_->disabled_rw(val); +} + inline bool enabled_ro() { return provider_->enabled_ro(); } +inline void enabled_ro(bool val) { + provider_->enabled_ro(val); +} + inline bool enabled_rw() { return provider_->enabled_rw(); } -inline void override_flag(std::string const& name, bool val) { - return provider_->override_flag(name, val); +inline void enabled_rw(bool val) { + provider_->enabled_rw(val); } -inline void reset_overrides() { - return provider_->reset_overrides(); +inline void reset_flags() { + return provider_->reset_flags(); } } @@ -306,28 +300,20 @@ public: using namespace server_configurable_flags; #include -#include -#include namespace com::android::aconfig::test { class flag_provider : public flag_provider_interface { private: std::unordered_map overrides_; - std::unordered_set flag_names_; public: flag_provider() - : overrides_(), - flag_names_() { - flag_names_.insert(DISABLED_RO); - flag_names_.insert(DISABLED_RW); - flag_names_.insert(ENABLED_RO); - flag_names_.insert(ENABLED_RW); - } + : overrides_() + {} virtual bool disabled_ro() override { - auto it = overrides_.find(DISABLED_RO); + auto it = overrides_.find("disabled_ro"); if (it != overrides_.end()) { return it->second; } else { @@ -335,8 +321,12 @@ public: } } + virtual void disabled_ro(bool val) override { + overrides_["disabled_ro"] = val; + } + virtual bool disabled_rw() override { - auto it = overrides_.find(DISABLED_RW); + auto it = overrides_.find("disabled_rw"); if (it != overrides_.end()) { return it->second; } else { @@ -347,8 +337,12 @@ public: } } + virtual void disabled_rw(bool val) override { + overrides_["disabled_rw"] = val; + } + virtual bool enabled_ro() override { - auto it = overrides_.find(ENABLED_RO); + auto it = overrides_.find("enabled_ro"); if (it != overrides_.end()) { return it->second; } else { @@ -356,8 +350,12 @@ public: } } + virtual void enabled_ro(bool val) override { + overrides_["enabled_ro"] = val; + } + virtual bool enabled_rw() override { - auto it = overrides_.find(ENABLED_RW); + auto it = overrides_.find("enabled_rw"); if (it != overrides_.end()) { return it->second; } else { @@ -368,12 +366,11 @@ public: } } - virtual void override_flag(std::string const& flag, bool val) override { - assert(flag_names_.count(flag)); - overrides_[flag] = val; + virtual void enabled_rw(bool val) override { + overrides_["enabled_rw"] = val; } - virtual void reset_overrides() override { + virtual void reset_flags() override { overrides_.clear(); } }; @@ -386,18 +383,12 @@ public: #include "com_android_aconfig_test_flag_provider.h" namespace com::android::aconfig::test { - - std::string const DISABLED_RO = "com.android.aconfig.test.disabled_ro"; - std::string const DISABLED_RW = "com.android.aconfig.test.disabled_rw"; - std::string const ENABLED_RO = "com.android.aconfig.test.enabled_ro"; - std::string const ENABLED_RW = "com.android.aconfig.test.enabled_rw"; - std::unique_ptr provider_ = std::make_unique(); } "#; - const C_EXPORTED_HEADER_EXPECTED: &str = r#" + const C_EXPORTED_PROD_HEADER_EXPECTED: &str = r#" #ifndef com_android_aconfig_test_c_HEADER_H #define com_android_aconfig_test_c_HEADER_H @@ -405,11 +396,6 @@ namespace com::android::aconfig::test { extern "C" { #endif -extern const char* com_android_aconfig_test_DISABLED_RO; -extern const char* com_android_aconfig_test_DISABLED_RW; -extern const char* com_android_aconfig_test_ENABLED_RO; -extern const char* com_android_aconfig_test_ENABLED_RW; - bool com_android_aconfig_test_disabled_ro(); bool com_android_aconfig_test_disabled_rw(); @@ -418,9 +404,37 @@ bool com_android_aconfig_test_enabled_ro(); bool com_android_aconfig_test_enabled_rw(); -void com_android_aconfig_test_override_flag(const char* name, bool val); +#ifdef __cplusplus +} +#endif +#endif +"#; -void com_android_aconfig_test_reset_overrides(); + const C_EXPORTED_TEST_HEADER_EXPECTED: &str = r#" +#ifndef com_android_aconfig_test_c_HEADER_H +#define com_android_aconfig_test_c_HEADER_H + +#ifdef __cplusplus +extern "C" { +#endif + +bool com_android_aconfig_test_disabled_ro(); + +void set_com_android_aconfig_test_disabled_ro(bool val); + +bool com_android_aconfig_test_disabled_rw(); + +void set_com_android_aconfig_test_disabled_rw(bool val); + +bool com_android_aconfig_test_enabled_ro(); + +void set_com_android_aconfig_test_enabled_ro(bool val); + +bool com_android_aconfig_test_enabled_rw(); + +void set_com_android_aconfig_test_enabled_rw(bool val); + +void com_android_aconfig_test_reset_flags(); #ifdef __cplusplus } @@ -428,15 +442,9 @@ void com_android_aconfig_test_reset_overrides(); #endif "#; - const C_SOURCE_FILE_EXPECTED: &str = r#" + const C_PROD_SOURCE_FILE_EXPECTED: &str = r#" #include "com_android_aconfig_test_c.h" #include "com_android_aconfig_test.h" -#include - -const char* com_android_aconfig_test_DISABLED_RO = "com.android.aconfig.test.disabled_ro"; -const char* com_android_aconfig_test_DISABLED_RW = "com.android.aconfig.test.disabled_rw"; -const char* com_android_aconfig_test_ENABLED_RO = "com.android.aconfig.test.enabled_ro"; -const char* com_android_aconfig_test_ENABLED_RW = "com.android.aconfig.test.enabled_rw"; bool com_android_aconfig_test_disabled_ro() { return com::android::aconfig::test::disabled_ro(); @@ -453,13 +461,46 @@ bool com_android_aconfig_test_enabled_ro() { bool com_android_aconfig_test_enabled_rw() { return com::android::aconfig::test::enabled_rw(); } +"#; -void com_android_aconfig_test_override_flag(const char* name, bool val) { - com::android::aconfig::test::override_flag(std::string(name), val); + const C_TEST_SOURCE_FILE_EXPECTED: &str = r#" +#include "com_android_aconfig_test_c.h" +#include "com_android_aconfig_test.h" + +bool com_android_aconfig_test_disabled_ro() { + return com::android::aconfig::test::disabled_ro(); } -void com_android_aconfig_test_reset_overrides() { - com::android::aconfig::test::reset_overrides(); +void set_com_android_aconfig_test_disabled_ro(bool val) { + com::android::aconfig::test::disabled_ro(val); +} + +bool com_android_aconfig_test_disabled_rw() { + return com::android::aconfig::test::disabled_rw(); +} + +void set_com_android_aconfig_test_disabled_rw(bool val) { + com::android::aconfig::test::disabled_rw(val); +} + +bool com_android_aconfig_test_enabled_ro() { + return com::android::aconfig::test::enabled_ro(); +} + +void set_com_android_aconfig_test_enabled_ro(bool val) { + com::android::aconfig::test::enabled_ro(val); +} + +bool com_android_aconfig_test_enabled_rw() { + return com::android::aconfig::test::enabled_rw(); +} + +void set_com_android_aconfig_test_enabled_rw(bool val) { + com::android::aconfig::test::enabled_rw(val); +} + +void com_android_aconfig_test_reset_flags() { + com::android::aconfig::test::reset_flags(); } "#; fn test_generate_cpp_code(mode: CodegenMode) { @@ -516,7 +557,10 @@ void com_android_aconfig_test_reset_overrides() { assert_eq!( None, crate::test::first_significant_code_diff( - C_EXPORTED_HEADER_EXPECTED, + match mode { + CodegenMode::Production => C_EXPORTED_PROD_HEADER_EXPECTED, + CodegenMode::Test => C_EXPORTED_TEST_HEADER_EXPECTED, + }, generated_files_map.get(&target_file_path).unwrap() ) ); @@ -526,7 +570,10 @@ void com_android_aconfig_test_reset_overrides() { assert_eq!( None, crate::test::first_significant_code_diff( - C_SOURCE_FILE_EXPECTED, + match mode { + CodegenMode::Production => C_PROD_SOURCE_FILE_EXPECTED, + CodegenMode::Test => C_TEST_SOURCE_FILE_EXPECTED, + }, generated_files_map.get(&target_file_path).unwrap() ) ); diff --git a/tools/aconfig/templates/c_exported_header.template b/tools/aconfig/templates/c_exported_header.template index 08af860305..c22b048dcf 100644 --- a/tools/aconfig/templates/c_exported_header.template +++ b/tools/aconfig/templates/c_exported_header.template @@ -5,16 +5,17 @@ extern "C" \{ #endif -{{ for item in class_elements}} -extern const char* {header}_{item.uppercase_flag_name}; -{{ endfor -}} +{{ for item in class_elements }} +bool {header}_{item.flag_name}(); -{{ for item in class_elements}} -bool {header}_{item.flag_name}();{{ endfor }} +{{ if for_test }} +void set_{header}_{item.flag_name}(bool val); +{{ -endif }} +{{ endfor - }} -void {header}_override_flag(const char* name, bool val); - -void {header}_reset_overrides(); +{{ if for_test }} +void {header}_reset_flags(); +{{ -endif }} #ifdef __cplusplus } diff --git a/tools/aconfig/templates/c_source_file.template b/tools/aconfig/templates/c_source_file.template index 18da29966e..3d2482d62e 100644 --- a/tools/aconfig/templates/c_source_file.template +++ b/tools/aconfig/templates/c_source_file.template @@ -1,21 +1,20 @@ #include "{header}_c.h" #include "{header}.h" -#include - -{{ for item in class_elements}} -const char* {header}_{item.uppercase_flag_name} = "{item.device_config_flag}"; -{{ endfor - }} {{ for item in class_elements}} bool {header}_{item.flag_name}() \{ return {cpp_namespace}::{item.flag_name}(); } -{{ endfor }} -void {header}_override_flag(const char* name, bool val) \{ - {cpp_namespace}::override_flag(std::string(name), val); +{{ if for_test }} +void set_{header}_{item.flag_name}(bool val) \{ + {cpp_namespace}::{item.flag_name}(val); } +{{ -endif }} +{{ endfor -}} -void {header}_reset_overrides() \{ - {cpp_namespace}::reset_overrides(); +{{ if for_test }} +void {header}_reset_flags() \{ + {cpp_namespace}::reset_flags(); } +{{ -endif }} diff --git a/tools/aconfig/templates/cpp_exported_header.template b/tools/aconfig/templates/cpp_exported_header.template index ee8f0dddac..e45548e9a3 100644 --- a/tools/aconfig/templates/cpp_exported_header.template +++ b/tools/aconfig/templates/cpp_exported_header.template @@ -11,35 +11,43 @@ public: virtual ~flag_provider_interface() = default; {{ for item in class_elements}} virtual bool {item.flag_name}() = 0; - {{ endfor }} - virtual void override_flag(std::string const&, bool) \{} - virtual void reset_overrides() \{} + {{ if for_test }} + virtual void {item.flag_name}(bool val) = 0; + {{ -endif }} + {{ endfor }} + + {{ if for_test }} + virtual void reset_flags() \{} + {{ -endif }} }; extern std::unique_ptr provider_; -{{ for item in class_elements}} -extern std::string const {item.uppercase_flag_name};{{ endfor }} + {{ for item in class_elements}} inline bool {item.flag_name}() \{ - {{ if for_prod }} + {{ if for_test }} + return provider_->{item.flag_name}(); + {{ -else- }} {{ if not item.readwrite- }} return {item.default_value}; {{ -else- }} return provider_->{item.flag_name}(); {{ -endif }} - {{ -else- }} - return provider_->{item.flag_name}(); {{ -endif }} } + +{{ if for_test }} +inline void {item.flag_name}(bool val) \{ + provider_->{item.flag_name}(val); +} +{{ -endif }} {{ endfor }} -inline void override_flag(std::string const& name, bool val) \{ - return provider_->override_flag(name, val); -} -inline void reset_overrides() \{ - return provider_->reset_overrides(); +{{ if for_test }} +inline void reset_flags() \{ + return provider_->reset_flags(); } - +{{ -endif }} } #endif diff --git a/tools/aconfig/templates/cpp_source_file.template b/tools/aconfig/templates/cpp_source_file.template index 1b4f336707..d267f03117 100644 --- a/tools/aconfig/templates/cpp_source_file.template +++ b/tools/aconfig/templates/cpp_source_file.template @@ -3,8 +3,6 @@ #include "{header}_flag_provider.h" namespace {cpp_namespace} \{ -{{ for item in class_elements}} -std::string const {item.uppercase_flag_name} = "{item.device_config_flag}";{{ endfor }} std::unique_ptr provider_ = std::make_unique(); } diff --git a/tools/aconfig/templates/cpp_test_flag_provider.template b/tools/aconfig/templates/cpp_test_flag_provider.template index a24116b7e1..afa56cff81 100644 --- a/tools/aconfig/templates/cpp_test_flag_provider.template +++ b/tools/aconfig/templates/cpp_test_flag_provider.template @@ -8,25 +8,20 @@ using namespace server_configurable_flags; {{ endif }} #include -#include -#include namespace {cpp_namespace} \{ class flag_provider : public flag_provider_interface \{ private: std::unordered_map overrides_; - std::unordered_set flag_names_; public: flag_provider() - : overrides_(), - flag_names_() \{ - {{ for item in class_elements}} - flag_names_.insert({item.uppercase_flag_name});{{ endfor }} - } + : overrides_() + \{} + {{ for item in class_elements}} virtual bool {item.flag_name}() override \{ - auto it = overrides_.find({item.uppercase_flag_name}); + auto it = overrides_.find("{item.flag_name}"); if (it != overrides_.end()) \{ return it->second; } else \{ @@ -40,13 +35,13 @@ public: {{ -endif }} } } - {{ endfor }} - virtual void override_flag(std::string const& flag, bool val) override \{ - assert(flag_names_.count(flag)); - overrides_[flag] = val; - } - virtual void reset_overrides() override \{ + virtual void {item.flag_name}(bool val) override \{ + overrides_["{item.flag_name}"] = val; + } + {{ endfor }} + + virtual void reset_flags() override \{ overrides_.clear(); } };