From 22b4ab86ef06bfa2b7bc3c960fa128998bfa639b Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Thu, 2 Jun 2022 14:12:07 +0200 Subject: [PATCH] [flags] Refactor MaybeBoolFlag to use base::Optional Use the existing {base::Optional} instead of the extra {MaybeBoolFlag} struct. This makes writing to a maybe-flag simpler because you just write a boolean value and that automatically initializes the optional. R=cbruni@chromium.org Bug: v8:12887 Change-Id: I940d20286d65ba4355dc04b4b6068a306706f295 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3686412 Reviewed-by: Camillo Bruni Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/main@{#80915} --- BUILD.gn | 5 +++- src/codegen/arm/assembler-arm.cc | 30 +++++++++---------- src/flags/flag-definitions.h | 24 +-------------- src/flags/flags.cc | 23 +++++++------- src/flags/flags.h | 1 + .../flags/flag-definitions-unittest.cc | 20 ++++++------- 6 files changed, 42 insertions(+), 61 deletions(-) diff --git a/BUILD.gn b/BUILD.gn index 5439051cf0..7800b39d30 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -2685,7 +2685,10 @@ v8_header_set("v8_flags") { "src/flags/flags.h", ] - deps = [ ":v8_shared_internal_headers" ] + deps = [ + ":v8_libbase", + ":v8_shared_internal_headers", + ] } v8_header_set("v8_internal_headers") { diff --git a/src/codegen/arm/assembler-arm.cc b/src/codegen/arm/assembler-arm.cc index 50661f52d9..38d682117f 100644 --- a/src/codegen/arm/assembler-arm.cc +++ b/src/codegen/arm/assembler-arm.cc @@ -81,9 +81,9 @@ static unsigned CpuFeaturesFromCommandLine() { // If any of the old (deprecated) flags are specified, print a warning, but // otherwise try to respect them for now. // TODO(jbramley): When all the old bots have been updated, remove this. - if (FLAG_enable_armv7.has_value || FLAG_enable_vfp3.has_value || - FLAG_enable_32dregs.has_value || FLAG_enable_neon.has_value || - FLAG_enable_sudiv.has_value || FLAG_enable_armv8.has_value) { + if (FLAG_enable_armv7.has_value() || FLAG_enable_vfp3.has_value() || + FLAG_enable_32dregs.has_value() || FLAG_enable_neon.has_value() || + FLAG_enable_sudiv.has_value() || FLAG_enable_armv8.has_value()) { // As an approximation of the old behaviour, set the default values from the // arm_arch setting, then apply the flags over the top. bool enable_armv7 = (result & (1u << ARMv7)) != 0; @@ -92,41 +92,41 @@ static unsigned CpuFeaturesFromCommandLine() { bool enable_neon = (result & (1u << ARMv7)) != 0; bool enable_sudiv = (result & (1u << ARMv7_SUDIV)) != 0; bool enable_armv8 = (result & (1u << ARMv8)) != 0; - if (FLAG_enable_armv7.has_value) { + if (FLAG_enable_armv7.has_value()) { fprintf(stderr, "Warning: --enable_armv7 is deprecated. " "Use --arm_arch instead.\n"); - enable_armv7 = FLAG_enable_armv7.value; + enable_armv7 = FLAG_enable_armv7.value(); } - if (FLAG_enable_vfp3.has_value) { + if (FLAG_enable_vfp3.has_value()) { fprintf(stderr, "Warning: --enable_vfp3 is deprecated. " "Use --arm_arch instead.\n"); - enable_vfp3 = FLAG_enable_vfp3.value; + enable_vfp3 = FLAG_enable_vfp3.value(); } - if (FLAG_enable_32dregs.has_value) { + if (FLAG_enable_32dregs.has_value()) { fprintf(stderr, "Warning: --enable_32dregs is deprecated. " "Use --arm_arch instead.\n"); - enable_32dregs = FLAG_enable_32dregs.value; + enable_32dregs = FLAG_enable_32dregs.value(); } - if (FLAG_enable_neon.has_value) { + if (FLAG_enable_neon.has_value()) { fprintf(stderr, "Warning: --enable_neon is deprecated. " "Use --arm_arch instead.\n"); - enable_neon = FLAG_enable_neon.value; + enable_neon = FLAG_enable_neon.value(); } - if (FLAG_enable_sudiv.has_value) { + if (FLAG_enable_sudiv.has_value()) { fprintf(stderr, "Warning: --enable_sudiv is deprecated. " "Use --arm_arch instead.\n"); - enable_sudiv = FLAG_enable_sudiv.value; + enable_sudiv = FLAG_enable_sudiv.value(); } - if (FLAG_enable_armv8.has_value) { + if (FLAG_enable_armv8.has_value()) { fprintf(stderr, "Warning: --enable_armv8 is deprecated. " "Use --arm_arch instead.\n"); - enable_armv8 = FLAG_enable_armv8.value; + enable_armv8 = FLAG_enable_armv8.value(); } // Emulate the old implications. if (enable_armv8) { diff --git a/src/flags/flag-definitions.h b/src/flags/flag-definitions.h index 5fae60f1da..6468ddbea5 100644 --- a/src/flags/flag-definitions.h +++ b/src/flags/flag-definitions.h @@ -122,26 +122,6 @@ #define DEFINE_NEG_VALUE_IMPLICATION(whenflag, thenflag, value) #endif -#define COMMA , - -#ifdef FLAG_MODE_DECLARE - -struct MaybeBoolFlag { - static MaybeBoolFlag Create(bool has_value, bool value) { - MaybeBoolFlag flag; - flag.has_value = has_value; - flag.value = value; - return flag; - } - bool has_value; - bool value; - - bool operator!=(const MaybeBoolFlag& other) const { - return has_value != other.has_value || value != other.value; - } -}; -#endif - #ifdef DEBUG #define DEBUG_BOOL true #else @@ -243,7 +223,7 @@ struct MaybeBoolFlag { #define DEFINE_BOOL_READONLY(nam, def, cmt) \ FLAG_READONLY(BOOL, bool, nam, def, cmt) #define DEFINE_MAYBE_BOOL(nam, cmt) \ - FLAG(MAYBE_BOOL, MaybeBoolFlag, nam, {false COMMA false}, cmt) + FLAG(MAYBE_BOOL, base::Optional, nam, base::nullopt, cmt) #define DEFINE_INT(nam, def, cmt) FLAG(INT, int, nam, def, cmt) #define DEFINE_UINT(nam, def, cmt) FLAG(UINT, unsigned int, nam, def, cmt) #define DEFINE_UINT_READONLY(nam, def, cmt) \ @@ -2316,5 +2296,3 @@ DEFINE_BOOL(enable_embedded_constant_pool, V8_EMBEDDED_CONSTANT_POOL, #undef FLAG_MODE_META #undef FLAG_MODE_DEFINE_IMPLICATIONS #undef FLAG_MODE_APPLY - -#undef COMMA diff --git a/src/flags/flags.cc b/src/flags/flags.cc index 60fae9bcd1..ad2e6c4c71 100644 --- a/src/flags/flags.cc +++ b/src/flags/flags.cc @@ -93,16 +93,17 @@ struct Flag { if (change_flag) *reinterpret_cast(valptr_) = value; } - MaybeBoolFlag maybe_bool_variable() const { + base::Optional maybe_bool_variable() const { DCHECK(type_ == TYPE_MAYBE_BOOL); - return *reinterpret_cast(valptr_); + return *reinterpret_cast*>(valptr_); } - void set_maybe_bool_variable(MaybeBoolFlag value, SetBy set_by) { + void set_maybe_bool_variable(base::Optional value, SetBy set_by) { DCHECK(type_ == TYPE_MAYBE_BOOL); - bool change_flag = *reinterpret_cast(valptr_) != value; + bool change_flag = + *reinterpret_cast*>(valptr_) != value; change_flag = CheckFlagChange(set_by, change_flag); - if (change_flag) *reinterpret_cast(valptr_) = value; + if (change_flag) *reinterpret_cast*>(valptr_) = value; } int int_variable() const { @@ -324,7 +325,7 @@ struct Flag { case TYPE_BOOL: return bool_variable() == bool_default(); case TYPE_MAYBE_BOOL: - return maybe_bool_variable().has_value == false; + return maybe_bool_variable().has_value() == false; case TYPE_INT: return int_variable() == int_default(); case TYPE_UINT: @@ -353,8 +354,7 @@ struct Flag { set_bool_variable(bool_default(), SetBy::kDefault); break; case TYPE_MAYBE_BOOL: - set_maybe_bool_variable(MaybeBoolFlag::Create(false, false), - SetBy::kDefault); + set_maybe_bool_variable(base::nullopt, SetBy::kDefault); break; case TYPE_INT: set_int_variable(int_default(), SetBy::kDefault); @@ -460,8 +460,8 @@ std::ostream& operator<<(std::ostream& os, const FlagValue& flag_value) { os << (flag.bool_variable() ? "true" : "false"); break; case Flag::TYPE_MAYBE_BOOL: - os << (flag.maybe_bool_variable().has_value - ? (flag.maybe_bool_variable().value ? "true" : "false") + os << (flag.maybe_bool_variable().has_value() + ? (flag.maybe_bool_variable().value() ? "true" : "false") : "unset"); break; case Flag::TYPE_INT: @@ -643,8 +643,7 @@ int FlagList::SetFlagsFromCommandLine(int* argc, char** argv, bool remove_flags, flag->set_bool_variable(!negated, Flag::SetBy::kCommandLine); break; case Flag::TYPE_MAYBE_BOOL: - flag->set_maybe_bool_variable(MaybeBoolFlag::Create(true, !negated), - Flag::SetBy::kCommandLine); + flag->set_maybe_bool_variable(!negated, Flag::SetBy::kCommandLine); break; case Flag::TYPE_INT: flag->set_int_variable(static_cast(strtol(value, &endp, 10)), diff --git a/src/flags/flags.h b/src/flags/flags.h index 8c247e2053..75d88f93e7 100644 --- a/src/flags/flags.h +++ b/src/flags/flags.h @@ -5,6 +5,7 @@ #ifndef V8_FLAGS_FLAGS_H_ #define V8_FLAGS_FLAGS_H_ +#include "src/base/optional.h" #include "src/common/globals.h" namespace v8 { diff --git a/test/unittests/flags/flag-definitions-unittest.cc b/test/unittests/flags/flag-definitions-unittest.cc index 6c39b5fd46..df1d45bfcd 100644 --- a/test/unittests/flags/flag-definitions-unittest.cc +++ b/test/unittests/flags/flag-definitions-unittest.cc @@ -69,8 +69,8 @@ TEST_F(FlagsTest, Flags2) { false)); CHECK_EQ(8, argc); CHECK(!FLAG_testing_bool_flag); - CHECK(FLAG_testing_maybe_bool_flag.has_value); - CHECK(!FLAG_testing_maybe_bool_flag.value); + CHECK(FLAG_testing_maybe_bool_flag.has_value()); + CHECK(!FLAG_testing_maybe_bool_flag.value()); CHECK_EQ(77, FLAG_testing_int_flag); CHECK_EQ(.25, FLAG_testing_float_flag); CHECK_EQ(0, strcmp(FLAG_testing_string_flag, "no way!")); @@ -85,8 +85,8 @@ TEST_F(FlagsTest, Flags2b) { "--testing_string_flag no_way! "; CHECK_EQ(0, FlagList::SetFlagsFromString(str, strlen(str))); CHECK(!FLAG_testing_bool_flag); - CHECK(FLAG_testing_maybe_bool_flag.has_value); - CHECK(!FLAG_testing_maybe_bool_flag.value); + CHECK(FLAG_testing_maybe_bool_flag.has_value()); + CHECK(!FLAG_testing_maybe_bool_flag.value()); CHECK_EQ(77, FLAG_testing_int_flag); CHECK_EQ(.25, FLAG_testing_float_flag); CHECK_EQ(0, strcmp(FLAG_testing_string_flag, "no_way!")); @@ -108,8 +108,8 @@ TEST_F(FlagsTest, Flags3) { true)); CHECK_EQ(2, argc); CHECK(FLAG_testing_bool_flag); - CHECK(FLAG_testing_maybe_bool_flag.has_value); - CHECK(FLAG_testing_maybe_bool_flag.value); + CHECK(FLAG_testing_maybe_bool_flag.has_value()); + CHECK(FLAG_testing_maybe_bool_flag.value()); CHECK_EQ(-666, FLAG_testing_int_flag); CHECK_EQ(-12E10, FLAG_testing_float_flag); CHECK_EQ(0, strcmp(FLAG_testing_string_flag, "foo-bar")); @@ -124,8 +124,8 @@ TEST_F(FlagsTest, Flags3b) { "-testing-string-flag=foo-bar"; CHECK_EQ(0, FlagList::SetFlagsFromString(str, strlen(str))); CHECK(FLAG_testing_bool_flag); - CHECK(FLAG_testing_maybe_bool_flag.has_value); - CHECK(FLAG_testing_maybe_bool_flag.value); + CHECK(FLAG_testing_maybe_bool_flag.has_value()); + CHECK(FLAG_testing_maybe_bool_flag.value()); CHECK_EQ(-666, FLAG_testing_int_flag); CHECK_EQ(-12E10, FLAG_testing_float_flag); CHECK_EQ(0, strcmp(FLAG_testing_string_flag, "foo-bar")); @@ -138,14 +138,14 @@ TEST_F(FlagsTest, Flags4) { CHECK_EQ(0, FlagList::SetFlagsFromCommandLine(&argc, const_cast(argv), true)); CHECK_EQ(2, argc); - CHECK(!FLAG_testing_maybe_bool_flag.has_value); + CHECK(!FLAG_testing_maybe_bool_flag.has_value()); } TEST_F(FlagsTest, Flags4b) { SetFlagsToDefault(); const char* str = "--testing_bool_flag --foo"; CHECK_EQ(2, FlagList::SetFlagsFromString(str, strlen(str))); - CHECK(!FLAG_testing_maybe_bool_flag.has_value); + CHECK(!FLAG_testing_maybe_bool_flag.has_value()); } TEST_F(FlagsTest, Flags5) {