From 46ea10f071fa6e4bdd6a5d6c789da02ee424f47c Mon Sep 17 00:00:00 2001 From: "dreamer.dead" Date: Fri, 29 Jul 2016 21:02:29 +0300 Subject: [PATCH 1/5] Do not pass flag type as a char literal when registering a new flag. It is possible to create a type-safe version of FlagRegisterer ctor (as well as some internal gflags classes), that will deduce type of the new flag automatically. This results in removing quite a few calls to strcmp() when new flag is created. No existing behavior change. --- src/gflags.cc | 124 ++++++++++++++++++++++++++-------------- src/gflags.h.in | 15 +++-- test/gflags_unittest.cc | 6 +- 3 files changed, 95 insertions(+), 50 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index 1619e9c..56414ec 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -191,17 +191,21 @@ static void ReportError(DieWhenReporting should_die, const char* format, ...) { class CommandLineFlag; class FlagValue { public: - FlagValue(void* valbuf, const char* type, bool transfer_ownership_of_value); + template + FlagValue(FlagType* valbuf, bool transfer_ownership_of_value); ~FlagValue(); bool ParseFrom(const char* spec); string ToString() const; + template + bool OfType() const { return type_ == FlagValueTraits::kValueType; } + private: friend class CommandLineFlag; // for many things, including Validate() friend class GFLAGS_NAMESPACE::FlagSaverImpl; // calls New() friend class FlagRegistry; // checks value_buffer_ for flags_by_ptr_ map - template friend T GetFromEnv(const char*, const char*, T); + template friend T GetFromEnv(const char*, T); friend bool TryParseLocked(const CommandLineFlag*, FlagValue*, const char*, string*); // for New(), CopyFrom() @@ -215,6 +219,10 @@ class FlagValue { FV_STRING = 6, FV_MAX_INDEX = 6, }; + + template + struct FlagValueTraits; + const char* TypeName() const; bool Equal(const FlagValue& x) const; FlagValue* New() const; // creates a new one with default value @@ -227,14 +235,33 @@ class FlagValue { // (*validate_fn)(bool) for a bool flag). bool Validate(const char* flagname, ValidateFnProto validate_fn_proto) const; - void* value_buffer_; // points to the buffer holding our data - int8 type_; // how to interpret value_ - bool owns_value_; // whether to free value on destruct + void* const value_buffer_; // points to the buffer holding our data + const int8 type_; // how to interpret value_ + const bool owns_value_; // whether to free value on destruct FlagValue(const FlagValue&); // no copying! void operator=(const FlagValue&); }; +// Map the given C++ type to a value of the ValueType enum at compile time. +#define DEFINE_FLAG_TRAITS(type, value) \ + template <> \ + struct FlagValue::FlagValueTraits { \ + static const ValueType kValueType = value; \ + } + +// Define full template specializations of the FlagValueTraits template +// for all supported flag types. +DEFINE_FLAG_TRAITS(bool, FV_BOOL); +DEFINE_FLAG_TRAITS(int32, FV_INT32); +DEFINE_FLAG_TRAITS(uint32, FV_UINT32); +DEFINE_FLAG_TRAITS(int64, FV_INT64); +DEFINE_FLAG_TRAITS(uint64, FV_UINT64); +DEFINE_FLAG_TRAITS(double, FV_DOUBLE); +DEFINE_FLAG_TRAITS(std::string, FV_STRING); + +#undef DEFINE_FLAG_TRAITS + // This could be a templated method of FlagValue, but doing so adds to the // size of the .o. Since there's no type-safety here anyway, macro is ok. @@ -242,15 +269,12 @@ class FlagValue { #define OTHER_VALUE_AS(fv, type) *reinterpret_cast(fv.value_buffer_) #define SET_VALUE_AS(type, value) VALUE_AS(type) = (value) -FlagValue::FlagValue(void* valbuf, const char* type, +template +FlagValue::FlagValue(FlagType* valbuf, bool transfer_ownership_of_value) : value_buffer_(valbuf), + type_(FlagValueTraits::kValueType), owns_value_(transfer_ownership_of_value) { - for (type_ = 0; type_ <= FV_MAX_INDEX; ++type_) { - if (!strcmp(type, TypeName())) { - break; - } - } assert(type_ <= FV_MAX_INDEX); // Unknown typename } @@ -438,15 +462,14 @@ bool FlagValue::Equal(const FlagValue& x) const { } FlagValue* FlagValue::New() const { - const char *type = TypeName(); switch (type_) { - case FV_BOOL: return new FlagValue(new bool(false), type, true); - case FV_INT32: return new FlagValue(new int32(0), type, true); - case FV_UINT32: return new FlagValue(new uint32(0), type, true); - case FV_INT64: return new FlagValue(new int64(0), type, true); - case FV_UINT64: return new FlagValue(new uint64(0), type, true); - case FV_DOUBLE: return new FlagValue(new double(0.0), type, true); - case FV_STRING: return new FlagValue(new string, type, true); + case FV_BOOL: return new FlagValue(new bool(false), true); + case FV_INT32: return new FlagValue(new int32(0), true); + case FV_UINT32: return new FlagValue(new uint32(0), true); + case FV_INT64: return new FlagValue(new int64(0), true); + case FV_UINT64: return new FlagValue(new uint64(0), true); + case FV_DOUBLE: return new FlagValue(new double(0.0), true); + case FV_STRING: return new FlagValue(new string, true); default: assert(false); return NULL; // unknown type } } @@ -510,6 +533,9 @@ class CommandLineFlag { ValidateFnProto validate_function() const { return validate_fn_proto_; } const void* flag_ptr() const { return current_->value_buffer_; } + template + bool OfType() const { return defvalue_->OfType(); } + void FillCommandLineFlagInfo(struct CommandLineFlagInfo* result); // If validate_fn_proto_ is non-NULL, calls it on value, returns result. @@ -800,7 +826,7 @@ CommandLineFlag* FlagRegistry::SplitArgumentLocked(const char* arg, kError, key->c_str()); return NULL; } - if (strcmp(flag->type_name(), "bool") != 0) { + if (!flag->OfType()) { // 'x' exists but is not boolean, so we're not in the exception case. *error_message = StringPrintf( "%sboolean value (%s) specified for %s command line flag\n", @@ -814,7 +840,7 @@ CommandLineFlag* FlagRegistry::SplitArgumentLocked(const char* arg, } // Assign a value if this is a boolean flag - if (*v == NULL && strcmp(flag->type_name(), "bool") == 0) { + if (*v == NULL && flag->OfType()) { *v = "1"; // the --nox case was already handled, so this is the --x case } @@ -1073,7 +1099,7 @@ uint32 CommandLineFlagParser::ParseNewCommandLineFlags(int* argc, char*** argv, if (value == NULL) { // Boolean options are always assigned a value by SplitArgumentLocked() - assert(strcmp(flag->type_name(), "bool") != 0); + assert(!flag->OfType()); if (i+1 >= first_nonopt) { // This flag needs a value, but there is nothing available error_flags_[key] = (string(kError) + "flag '" + (*argv)[i] + "'" @@ -1098,7 +1124,7 @@ uint32 CommandLineFlagParser::ParseNewCommandLineFlags(int* argc, char*** argv, // "-lat -30.5" would trigger the warning. The common cases we // want to solve talk about true and false as values. if (value[0] == '-' - && strcmp(flag->type_name(), "string") == 0 + && flag->OfType() && (strstr(flag->help(), "true") || strstr(flag->help(), "false"))) { LOG(WARNING) << "Did you really mean to set flag '" @@ -1163,8 +1189,8 @@ string CommandLineFlagParser::ProcessFromenvLocked(const string& flagval, } const string envname = string("FLAGS_") + string(flagname); - string envval; - if (!SafeGetEnv(envname.c_str(), envval)) { + string envval; + if (!SafeGetEnv(envname.c_str(), envval)) { if (errors_are_fatal) { error_flags_[flagname] = (string(kError) + envname + " not found in environment\n"); @@ -1362,14 +1388,14 @@ string CommandLineFlagParser::ProcessOptionsFromStringLocked( // -------------------------------------------------------------------- template -T GetFromEnv(const char *varname, const char* type, T dflt) { +T GetFromEnv(const char *varname, T dflt) { std::string valstr; if (SafeGetEnv(varname, valstr)) { - FlagValue ifv(new T, type, true); + FlagValue ifv(new T, true); if (!ifv.ParseFrom(valstr.c_str())) { ReportError(DIE, "ERROR: error parsing env variable '%s' with value '%s'\n", varname, valstr.c_str()); - } + } return OTHER_VALUE_AS(ifv, T); } else return dflt; } @@ -1416,23 +1442,37 @@ bool AddFlagValidator(const void* flag_ptr, ValidateFnProto validate_fn_proto) { // values in a global destructor. // -------------------------------------------------------------------- -FlagRegisterer::FlagRegisterer(const char* name, const char* type, +template +FlagRegisterer::FlagRegisterer(const char* name, const char* help, const char* filename, - void* current_storage, void* defvalue_storage) { + FlagType* current_storage, FlagType* defvalue_storage) { if (help == NULL) help = ""; - // FlagValue expects the type-name to not include any namespace - // components, so we get rid of those, if any. - if (strchr(type, ':')) - type = strrchr(type, ':') + 1; - FlagValue* current = new FlagValue(current_storage, type, false); - FlagValue* defvalue = new FlagValue(defvalue_storage, type, false); + FlagValue* current = new FlagValue(current_storage, false); + FlagValue* defvalue = new FlagValue(defvalue_storage, false); // Importantly, flag_ will never be deleted, so storage is always good. CommandLineFlag* flag = new CommandLineFlag(name, help, filename, current, defvalue); FlagRegistry::GlobalRegistry()->RegisterFlag(flag); // default registry } +// Force compiler to generate code for the given template specialization. +#define INSTANTIATE_FLAG_REGISTERER_CTOR(type) \ + template FlagRegisterer::FlagRegisterer( \ + const char* name, const char* help, const char* filename, \ + type* current_storage, type* defvalue_storage) + +// Do this for all supported flag types. +INSTANTIATE_FLAG_REGISTERER_CTOR(bool); +INSTANTIATE_FLAG_REGISTERER_CTOR(int32); +INSTANTIATE_FLAG_REGISTERER_CTOR(uint32); +INSTANTIATE_FLAG_REGISTERER_CTOR(int64); +INSTANTIATE_FLAG_REGISTERER_CTOR(uint64); +INSTANTIATE_FLAG_REGISTERER_CTOR(double); +INSTANTIATE_FLAG_REGISTERER_CTOR(std::string); + +#undef INSTANTIATE_FLAG_REGISTERER_CTOR + // -------------------------------------------------------------------- // GetAllFlags() // The main way the FlagRegistry class exposes its data. This @@ -1820,22 +1860,22 @@ bool ReadFromFlagsFile(const string& filename, const char* prog_name, // -------------------------------------------------------------------- bool BoolFromEnv(const char *v, bool dflt) { - return GetFromEnv(v, "bool", dflt); + return GetFromEnv(v, dflt); } int32 Int32FromEnv(const char *v, int32 dflt) { - return GetFromEnv(v, "int32", dflt); + return GetFromEnv(v, dflt); } uint32 Uint32FromEnv(const char *v, uint32 dflt) { - return GetFromEnv(v, "uint32", dflt); + return GetFromEnv(v, dflt); } int64 Int64FromEnv(const char *v, int64 dflt) { - return GetFromEnv(v, "int64", dflt); + return GetFromEnv(v, dflt); } uint64 Uint64FromEnv(const char *v, uint64 dflt) { - return GetFromEnv(v, "uint64", dflt); + return GetFromEnv(v, dflt); } double DoubleFromEnv(const char *v, double dflt) { - return GetFromEnv(v, "double", dflt); + return GetFromEnv(v, dflt); } #ifdef _MSC_VER diff --git a/src/gflags.h.in b/src/gflags.h.in index 03b7776..09daac3 100644 --- a/src/gflags.h.in +++ b/src/gflags.h.in @@ -431,9 +431,14 @@ extern GFLAGS_DLL_DECL void ShutDownCommandLineFlags(); class GFLAGS_DLL_DECL FlagRegisterer { public: - FlagRegisterer(const char* name, const char* type, + // We instantiate this template ctor for all supported types, + // so it is possible to place implementation of the FlagRegisterer ctor in + // .cc file. + // Calling this constructor with unsupported type will produce linker error. + template + FlagRegisterer(const char* name, const char* help, const char* filename, - void* current_storage, void* defvalue_storage); + FlagType* current_storage, FlagType* defvalue_storage); }; // If your application #defines STRIP_FLAG_HELP to a non-zero value @@ -475,7 +480,7 @@ extern GFLAGS_DLL_DECL const char kStrippedFlagHelp[]; GFLAGS_DLL_DEFINE_FLAG type FLAGS_##name = FLAGS_nono##name; \ type FLAGS_no##name = FLAGS_nono##name; \ static GFLAGS_NAMESPACE::FlagRegisterer o_##name( \ - #name, #type, MAYBE_STRIPPED_HELP(help), __FILE__, \ + #name, MAYBE_STRIPPED_HELP(help), __FILE__, \ &FLAGS_##name, &FLAGS_no##name); \ } \ using fL##shorttype::FLAGS_##name @@ -581,8 +586,8 @@ public: dont_pass0toDEFINE_string(s_##name[0].s, \ val); \ static GFLAGS_NAMESPACE::FlagRegisterer o_##name( \ - #name, "string", MAYBE_STRIPPED_HELP(txt), __FILE__, \ - s_##name[0].s, new (s_##name[1].s) clstring(*FLAGS_no##name)); \ + #name, MAYBE_STRIPPED_HELP(txt), __FILE__, \ + FLAGS_no##name, new (s_##name[1].s) clstring(*FLAGS_no##name)); \ static StringFlagDestructor d_##name(s_##name[0].s, s_##name[1].s); \ extern GFLAGS_DLL_DEFINE_FLAG clstring& FLAGS_##name; \ using fLS::FLAGS_##name; \ diff --git a/test/gflags_unittest.cc b/test/gflags_unittest.cc index 4c079ba..47dfd3c 100755 --- a/test/gflags_unittest.cc +++ b/test/gflags_unittest.cc @@ -216,7 +216,7 @@ namespace fLI { int32 FLAGS_tldflag1 = FLAGS_nonotldflag1; int32 FLAGS_notldflag1 = FLAGS_nonotldflag1; static FlagRegisterer o_tldflag1( - "tldflag1", "int32", + "tldflag1", "should show up in --helpshort", "gflags_unittest.cc", &FLAGS_tldflag1, &FLAGS_notldflag1); } @@ -227,7 +227,7 @@ namespace fLI { int32 FLAGS_tldflag2 = FLAGS_nonotldflag2; int32 FLAGS_notldflag2 = FLAGS_nonotldflag2; static FlagRegisterer o_tldflag2( - "tldflag2", "int32", + "tldflag2", "should show up in --helpshort", "gflags_unittest.", &FLAGS_tldflag2, &FLAGS_notldflag2); } @@ -1355,7 +1355,7 @@ TEST(ParseCommandLineFlagsWrongFields, // addresses of these variables will be overwritten... Stack smash! static bool current_storage; static bool defvalue_storage; - FlagRegisterer fr("flag_name", "bool", 0, "filename", + FlagRegisterer fr("flag_name", NULL, "filename", ¤t_storage, &defvalue_storage); CommandLineFlagInfo fi; EXPECT_TRUE(GetCommandLineFlagInfo("flag_name", &fi)); From a1e461d61d5a6fbc593426d595d9d3dd8a6d83c0 Mon Sep 17 00:00:00 2001 From: "dreamer.dead" Date: Mon, 1 Aug 2016 14:51:11 +0300 Subject: [PATCH 2/5] Change template FlagValue::OfType() to Type() getter. --- src/gflags.cc | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index 56414ec..c98b614 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -191,24 +191,6 @@ static void ReportError(DieWhenReporting should_die, const char* format, ...) { class CommandLineFlag; class FlagValue { public: - template - FlagValue(FlagType* valbuf, bool transfer_ownership_of_value); - ~FlagValue(); - - bool ParseFrom(const char* spec); - string ToString() const; - - template - bool OfType() const { return type_ == FlagValueTraits::kValueType; } - - private: - friend class CommandLineFlag; // for many things, including Validate() - friend class GFLAGS_NAMESPACE::FlagSaverImpl; // calls New() - friend class FlagRegistry; // checks value_buffer_ for flags_by_ptr_ map - template friend T GetFromEnv(const char*, T); - friend bool TryParseLocked(const CommandLineFlag*, FlagValue*, - const char*, string*); // for New(), CopyFrom() - enum ValueType { FV_BOOL = 0, FV_INT32 = 1, @@ -220,6 +202,23 @@ class FlagValue { FV_MAX_INDEX = 6, }; + template + FlagValue(FlagType* valbuf, bool transfer_ownership_of_value); + ~FlagValue(); + + bool ParseFrom(const char* spec); + string ToString() const; + + ValueType Type() const { return static_cast(type_); } + + private: + friend class CommandLineFlag; // for many things, including Validate() + friend class GFLAGS_NAMESPACE::FlagSaverImpl; // calls New() + friend class FlagRegistry; // checks value_buffer_ for flags_by_ptr_ map + template friend T GetFromEnv(const char*, T); + friend bool TryParseLocked(const CommandLineFlag*, FlagValue*, + const char*, string*); // for New(), CopyFrom() + template struct FlagValueTraits; @@ -533,8 +532,7 @@ class CommandLineFlag { ValidateFnProto validate_function() const { return validate_fn_proto_; } const void* flag_ptr() const { return current_->value_buffer_; } - template - bool OfType() const { return defvalue_->OfType(); } + FlagValue::ValueType Type() const { return defvalue_->Type(); } void FillCommandLineFlagInfo(struct CommandLineFlagInfo* result); @@ -826,7 +824,7 @@ CommandLineFlag* FlagRegistry::SplitArgumentLocked(const char* arg, kError, key->c_str()); return NULL; } - if (!flag->OfType()) { + if (flag->Type() != FlagValue::FV_BOOL) { // 'x' exists but is not boolean, so we're not in the exception case. *error_message = StringPrintf( "%sboolean value (%s) specified for %s command line flag\n", @@ -840,7 +838,7 @@ CommandLineFlag* FlagRegistry::SplitArgumentLocked(const char* arg, } // Assign a value if this is a boolean flag - if (*v == NULL && flag->OfType()) { + if (*v == NULL && flag->Type() == FlagValue::FV_BOOL) { *v = "1"; // the --nox case was already handled, so this is the --x case } @@ -1124,7 +1122,7 @@ uint32 CommandLineFlagParser::ParseNewCommandLineFlags(int* argc, char*** argv, // "-lat -30.5" would trigger the warning. The common cases we // want to solve talk about true and false as values. if (value[0] == '-' - && flag->OfType() + && flag->Type() != FlagValue::FV_STRING && (strstr(flag->help(), "true") || strstr(flag->help(), "false"))) { LOG(WARNING) << "Did you really mean to set flag '" From 3c0ad4fc9e0f2861011036a2bee4174cecb84303 Mon Sep 17 00:00:00 2001 From: "dreamer.dead" Date: Mon, 1 Aug 2016 14:52:26 +0300 Subject: [PATCH 3/5] Extract common code from FlagRegisterer to reduce size. --- src/gflags.cc | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index c98b614..d0b3e47 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -1440,18 +1440,30 @@ bool AddFlagValidator(const void* flag_ptr, ValidateFnProto validate_fn_proto) { // values in a global destructor. // -------------------------------------------------------------------- -template -FlagRegisterer::FlagRegisterer(const char* name, - const char* help, const char* filename, - FlagType* current_storage, FlagType* defvalue_storage) { +namespace { +void RegisterCommandLineFlag(const char* name, + const char* help, + const char* filename, + FlagValue* current, + FlagValue* defvalue) { if (help == NULL) help = ""; - FlagValue* current = new FlagValue(current_storage, false); - FlagValue* defvalue = new FlagValue(defvalue_storage, false); // Importantly, flag_ will never be deleted, so storage is always good. - CommandLineFlag* flag = new CommandLineFlag(name, help, filename, - current, defvalue); - FlagRegistry::GlobalRegistry()->RegisterFlag(flag); // default registry + CommandLineFlag* flag = + new CommandLineFlag(name, help, filename, current, defvalue); + FlagRegistry::GlobalRegistry()->RegisterFlag(flag); // default registry +} +} + +template +FlagRegisterer::FlagRegisterer(const char* name, + const char* help, + const char* filename, + FlagType* current_storage, + FlagType* defvalue_storage) { + FlagValue* const current = new FlagValue(current_storage, false); + FlagValue* const defvalue = new FlagValue(defvalue_storage, false); + RegisterCommandLineFlag(name, help, filename, current, defvalue); } // Force compiler to generate code for the given template specialization. From 30519426c32774f3a31c6598ae8ca49b4d1c62f2 Mon Sep 17 00:00:00 2001 From: "dreamer.dead" Date: Mon, 1 Aug 2016 14:54:46 +0300 Subject: [PATCH 4/5] Fix indentation and remove outdated assert. --- src/gflags.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index d0b3e47..7a8af21 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -274,7 +274,6 @@ FlagValue::FlagValue(FlagType* valbuf, : value_buffer_(valbuf), type_(FlagValueTraits::kValueType), owns_value_(transfer_ownership_of_value) { - assert(type_ <= FV_MAX_INDEX); // Unknown typename } FlagValue::~FlagValue() { @@ -1187,8 +1186,8 @@ string CommandLineFlagParser::ProcessFromenvLocked(const string& flagval, } const string envname = string("FLAGS_") + string(flagname); - string envval; - if (!SafeGetEnv(envname.c_str(), envval)) { + string envval; + if (!SafeGetEnv(envname.c_str(), envval)) { if (errors_are_fatal) { error_flags_[flagname] = (string(kError) + envname + " not found in environment\n"); @@ -1393,7 +1392,7 @@ T GetFromEnv(const char *varname, T dflt) { if (!ifv.ParseFrom(valstr.c_str())) { ReportError(DIE, "ERROR: error parsing env variable '%s' with value '%s'\n", varname, valstr.c_str()); - } + } return OTHER_VALUE_AS(ifv, T); } else return dflt; } From 7ba9921866f683f19f1aa43ce07fa542401ebe16 Mon Sep 17 00:00:00 2001 From: "dreamer.dead" Date: Mon, 1 Aug 2016 16:45:49 +0300 Subject: [PATCH 5/5] Fix wrong type comparison and outdated OfType() usage. --- src/gflags.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index 7a8af21..bbd58ec 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -1096,7 +1096,7 @@ uint32 CommandLineFlagParser::ParseNewCommandLineFlags(int* argc, char*** argv, if (value == NULL) { // Boolean options are always assigned a value by SplitArgumentLocked() - assert(!flag->OfType()); + assert(flag->Type() != FlagValue::FV_BOOL); if (i+1 >= first_nonopt) { // This flag needs a value, but there is nothing available error_flags_[key] = (string(kError) + "flag '" + (*argv)[i] + "'" @@ -1121,7 +1121,7 @@ uint32 CommandLineFlagParser::ParseNewCommandLineFlags(int* argc, char*** argv, // "-lat -30.5" would trigger the warning. The common cases we // want to solve talk about true and false as values. if (value[0] == '-' - && flag->Type() != FlagValue::FV_STRING + && flag->Type() == FlagValue::FV_STRING && (strstr(flag->help(), "true") || strstr(flag->help(), "false"))) { LOG(WARNING) << "Did you really mean to set flag '"