diff options
Diffstat (limited to 'absl/strings')
-rw-r--r-- | absl/strings/cord.cc | 18 | ||||
-rw-r--r-- | absl/strings/cordz_test.cc | 99 | ||||
-rw-r--r-- | absl/strings/internal/cord_rep_ring.cc | 83 | ||||
-rw-r--r-- | absl/strings/internal/cord_rep_ring.h | 37 | ||||
-rw-r--r-- | absl/strings/internal/cordz_functions.cc | 10 | ||||
-rw-r--r-- | absl/strings/internal/cordz_functions_test.cc | 18 | ||||
-rw-r--r-- | absl/strings/internal/cordz_info.cc | 10 | ||||
-rw-r--r-- | absl/strings/internal/cordz_info.h | 43 | ||||
-rw-r--r-- | absl/strings/internal/cordz_info_test.cc | 52 | ||||
-rw-r--r-- | absl/strings/internal/str_format/bind.cc | 7 | ||||
-rw-r--r-- | absl/strings/internal/str_format/extension.cc | 12 | ||||
-rw-r--r-- | absl/strings/internal/str_format/extension.h | 54 | ||||
-rw-r--r-- | absl/strings/internal/str_format/parser.cc | 135 | ||||
-rw-r--r-- | absl/strings/internal/str_format/parser.h | 40 | ||||
-rw-r--r-- | absl/strings/internal/str_format/parser_test.cc | 23 |
15 files changed, 377 insertions, 264 deletions
diff --git a/absl/strings/cord.cc b/absl/strings/cord.cc index 5dad781e..f5aa6e47 100644 --- a/absl/strings/cord.cc +++ b/absl/strings/cord.cc @@ -536,24 +536,10 @@ void Cord::InlineRep::AssignSlow(const Cord::InlineRep& src) { return; } - // See b/187581164: unsample cord if already sampled - // TODO(b/117940323): continuously 'assigned to' cords would reach 100% - // sampling probability. Imagine a cord x in some cache: - // cache.SetCord(const Cord& foo) { - // x = foo; - // } - // CordzInfo::MaybeTrackCord does: - // x.profiled = foo.profiled | x.profiled | random(cordz_mean_interval) - // Which means it will on the long run converge to 'always samples' - // The real fix is in CordzMaybeTrackCord, but the below is a low risk - // forward fix for b/187581164 and similar BT benchmark regressions. - if (ABSL_PREDICT_FALSE(is_profiled())) { - cordz_info()->Untrack(); - clear_cordz_info(); - } - CordRep* tree = as_tree(); if (CordRep* src_tree = src.tree()) { + // Leave any existing `cordz_info` in place, and let MaybeTrackCord() + // decide if this cord should be (or remains to be) sampled or not. data_.set_tree(CordRep::Ref(src_tree)); CordzInfo::MaybeTrackCord(data_, src.data_, method); } else { diff --git a/absl/strings/cordz_test.cc b/absl/strings/cordz_test.cc index 0e11f5c8..2b7d30b0 100644 --- a/absl/strings/cordz_test.cc +++ b/absl/strings/cordz_test.cc @@ -69,6 +69,7 @@ absl::string_view MakeString(TestCordSize size) { // Returns a cord with a sampled method of kAppendString. absl::Cord MakeAppendStringCord(TestCordSize size) { + CordzSamplingIntervalHelper always(1); absl::Cord cord; cord.Append(MakeString(size)); return cord; @@ -136,21 +137,21 @@ TEST_P(CordzStringTest, ConstructString) { } } -TEST(CordzTest, CopyConstruct) { +TEST(CordzTest, CopyConstructFromUnsampled) { CordzSamplingIntervalHelper sample_every{1}; Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); Cord cord(src); - EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorCord)); + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); } TEST(CordzTest, CopyConstructFromSampled) { - CordzSamplingIntervalHelper sample_every{1}; - Cord src(MakeString(TestCordSize::kLarge)); + CordzSamplingIntervalHelper sample_never{99999}; + Cord src = MakeAppendStringCord(TestCordSize::kLarge); Cord cord(src); ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorCord)); CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics(); - EXPECT_THAT(stats.parent_method, Eq(Method::kConstructorString)); - EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(1)); + EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString)); + EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1)); } TEST(CordzTest, MoveConstruct) { @@ -160,12 +161,12 @@ TEST(CordzTest, MoveConstruct) { EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } -TEST_P(CordzUpdateTest, AssignCord) { +TEST_P(CordzUpdateTest, AssignUnsampledCord) { Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); + const CordzInfo* info = GetCordzInfoForTesting(cord()); cord() = src; - EXPECT_THAT(cord(), HasValidCordzInfoOf(Method::kAssignCord)); - CordzStatistics stats = GetCordzInfoForTesting(cord())->GetCordzStatistics(); - EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0)); + EXPECT_THAT(GetCordzInfoForTesting(cord()), Eq(nullptr)); + EXPECT_FALSE(CordzInfoIsListed(info)); } TEST_P(CordzUpdateTest, AssignSampledCord) { @@ -178,10 +179,22 @@ TEST_P(CordzUpdateTest, AssignSampledCord) { EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0)); } -TEST(CordzUpdateTest, AssignSampledCordToUnsampledCord) { - CordzSamplingIntervalHelper sample_every{1}; +TEST(CordzUpdateTest, AssignSampledCordToInlined) { + CordzSamplingIntervalHelper sample_never{99999}; + Cord cord; Cord src = MakeAppendStringCord(TestCordSize::kLarge); + cord = src; + ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kAssignCord)); + CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics(); + EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString)); + EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1)); + EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0)); +} + +TEST(CordzUpdateTest, AssignSampledCordToUnsampledCord) { + CordzSamplingIntervalHelper sample_never{99999}; Cord cord = UnsampledCord(MakeString(TestCordSize::kLarge)); + Cord src = MakeAppendStringCord(TestCordSize::kLarge); cord = src; ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kAssignCord)); CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics(); @@ -190,6 +203,26 @@ TEST(CordzUpdateTest, AssignSampledCordToUnsampledCord) { EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0)); } +TEST(CordzUpdateTest, AssignUnsampledCordToSampledCordWithoutSampling) { + CordzSamplingIntervalHelper sample_never{99999}; + Cord cord = MakeAppendStringCord(TestCordSize::kLarge); + const CordzInfo* info = GetCordzInfoForTesting(cord); + Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); + cord = src; + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); + EXPECT_FALSE(CordzInfoIsListed(info)); +} + +TEST(CordzUpdateTest, AssignUnsampledCordToSampledCordWithSampling) { + CordzSamplingIntervalHelper sample_every{1}; + Cord cord = MakeAppendStringCord(TestCordSize::kLarge); + const CordzInfo* info = GetCordzInfoForTesting(cord); + Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); + cord = src; + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); + EXPECT_FALSE(CordzInfoIsListed(info)); +} + TEST(CordzUpdateTest, AssignSampledCordToSampledCord) { CordzSamplingIntervalHelper sample_every{1}; Cord src = MakeAppendStringCord(TestCordSize::kLarge); @@ -202,7 +235,19 @@ TEST(CordzUpdateTest, AssignSampledCordToSampledCord) { EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0)); } -TEST(CordzTest, AssignInlinedCord) { +TEST(CordzUpdateTest, AssignUnsampledCordToSampledCord) { + CordzSamplingIntervalHelper sample_every{1}; + Cord src = MakeAppendStringCord(TestCordSize::kLarge); + Cord cord(MakeString(TestCordSize::kLarge)); + cord = src; + ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kAssignCord)); + CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics(); + EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString)); + EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1)); + EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0)); +} + +TEST(CordzTest, AssignInlinedCordToSampledCord) { CordzSampleToken token; CordzSamplingIntervalHelper sample_every{1}; Cord cord(MakeString(TestCordSize::kLarge)); @@ -389,28 +434,28 @@ TEST(CordzTest, RemoveSuffix) { EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); } -TEST(CordzTest, SubCord) { +TEST(CordzTest, SubCordFromUnsampledCord) { CordzSamplingIntervalHelper sample_every{1}; Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); Cord cord = src.Subcord(10, src.size() / 2); - EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord)); -} - -TEST(CordzTest, SmallSubCord) { - CordzSamplingIntervalHelper sample_every{1}; - Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); - Cord cord = src.Subcord(10, kMaxInline + 1); - EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord)); + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); } TEST(CordzTest, SubCordFromSampledCord) { - CordzSamplingIntervalHelper sample_every{1}; - Cord src(MakeString(TestCordSize::kLarge)); + CordzSamplingIntervalHelper sample_never{99999}; + Cord src = MakeAppendStringCord(TestCordSize::kLarge); Cord cord = src.Subcord(10, src.size() / 2); - EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord)); + ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord)); CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics(); - EXPECT_THAT(stats.parent_method, Eq(Method::kConstructorString)); - EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(1)); + EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString)); + EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1)); +} + +TEST(CordzTest, SmallSubCord) { + CordzSamplingIntervalHelper sample_never{99999}; + Cord src = MakeAppendStringCord(TestCordSize::kLarge); + Cord cord = src.Subcord(10, kMaxInline + 1); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord)); } } // namespace diff --git a/absl/strings/internal/cord_rep_ring.cc b/absl/strings/internal/cord_rep_ring.cc index 09951290..f78c94e1 100644 --- a/absl/strings/internal/cord_rep_ring.cc +++ b/absl/strings/internal/cord_rep_ring.cc @@ -32,15 +32,6 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace cord_internal { -// See https://bugs.llvm.org/show_bug.cgi?id=48477 -#ifdef __clang__ -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wshadow" -#if __has_warning("-Wshadow-field") -#pragma clang diagnostic ignored "-Wshadow-field" -#endif -#endif - namespace { using index_type = CordRepRing::index_type; @@ -450,12 +441,12 @@ Span<char> CordRepRing::GetPrependBuffer(size_t size) { } CordRepRing* CordRepRing::CreateFromLeaf(CordRep* child, size_t offset, - size_t length, size_t extra) { + size_t len, size_t extra) { CordRepRing* rep = CordRepRing::New(1, extra); rep->head_ = 0; rep->tail_ = rep->advance(0); - rep->length = length; - rep->entry_end_pos()[0] = length; + rep->length = len; + rep->entry_end_pos()[0] = len; rep->entry_child()[0] = child; rep->entry_data_offset()[0] = static_cast<offset_type>(offset); return Validate(rep); @@ -463,16 +454,16 @@ CordRepRing* CordRepRing::CreateFromLeaf(CordRep* child, size_t offset, CordRepRing* CordRepRing::CreateSlow(CordRep* child, size_t extra) { CordRepRing* rep = nullptr; - Consume(child, [&](CordRep* child, size_t offset, size_t length) { - if (IsFlatOrExternal(child)) { - rep = rep ? AppendLeaf(rep, child, offset, length) - : CreateFromLeaf(child, offset, length, extra); + Consume(child, [&](CordRep* child_arg, size_t offset, size_t len) { + if (IsFlatOrExternal(child_arg)) { + rep = rep ? AppendLeaf(rep, child_arg, offset, len) + : CreateFromLeaf(child_arg, offset, len, extra); } else if (rep) { - rep = AddRing<AddMode::kAppend>(rep, child->ring(), offset, length); - } else if (offset == 0 && child->length == length) { - rep = Mutable(child->ring(), extra); + rep = AddRing<AddMode::kAppend>(rep, child_arg->ring(), offset, len); + } else if (offset == 0 && child_arg->length == len) { + rep = Mutable(child_arg->ring(), extra); } else { - rep = SubRing(child->ring(), offset, length, extra); + rep = SubRing(child_arg->ring(), offset, len, extra); } }); return Validate(rep, nullptr, __LINE__); @@ -491,18 +482,18 @@ CordRepRing* CordRepRing::Create(CordRep* child, size_t extra) { template <CordRepRing::AddMode mode> CordRepRing* CordRepRing::AddRing(CordRepRing* rep, CordRepRing* ring, - size_t offset, size_t length) { + size_t offset, size_t len) { assert(offset < ring->length); constexpr bool append = mode == AddMode::kAppend; Position head = ring->Find(offset); - Position tail = ring->FindTail(head.index, offset + length); + Position tail = ring->FindTail(head.index, offset + len); const index_type entries = ring->entries(head.index, tail.index); rep = Mutable(rep, entries); // The delta for making ring[head].end_pos into 'len - offset' const pos_type delta_length = - (append ? rep->begin_pos_ + rep->length : rep->begin_pos_ - length) - + (append ? rep->begin_pos_ + rep->length : rep->begin_pos_ - len) - ring->entry_begin_pos(head.index) - head.offset; // Start filling at `tail`, or `entries` before `head` @@ -543,36 +534,36 @@ CordRepRing* CordRepRing::AddRing(CordRepRing* rep, CordRepRing* ring, } // Commit changes - rep->length += length; + rep->length += len; if (append) { rep->tail_ = filler.pos(); } else { rep->head_ = filler.head(); - rep->begin_pos_ -= length; + rep->begin_pos_ -= len; } return Validate(rep); } CordRepRing* CordRepRing::AppendSlow(CordRepRing* rep, CordRep* child) { - Consume(child, [&rep](CordRep* child, size_t offset, size_t length) { - if (child->tag == RING) { - rep = AddRing<AddMode::kAppend>(rep, child->ring(), offset, length); + Consume(child, [&rep](CordRep* child_arg, size_t offset, size_t len) { + if (child_arg->tag == RING) { + rep = AddRing<AddMode::kAppend>(rep, child_arg->ring(), offset, len); } else { - rep = AppendLeaf(rep, child, offset, length); + rep = AppendLeaf(rep, child_arg, offset, len); } }); return rep; } CordRepRing* CordRepRing::AppendLeaf(CordRepRing* rep, CordRep* child, - size_t offset, size_t length) { + size_t offset, size_t len) { rep = Mutable(rep, 1); index_type back = rep->tail_; const pos_type begin_pos = rep->begin_pos_ + rep->length; rep->tail_ = rep->advance(rep->tail_); - rep->length += length; - rep->entry_end_pos()[back] = begin_pos + length; + rep->length += len; + rep->entry_end_pos()[back] = begin_pos + len; rep->entry_child()[back] = child; rep->entry_data_offset()[back] = static_cast<offset_type>(offset); return Validate(rep, nullptr, __LINE__); @@ -590,24 +581,24 @@ CordRepRing* CordRepRing::Append(CordRepRing* rep, CordRep* child) { } CordRepRing* CordRepRing::PrependSlow(CordRepRing* rep, CordRep* child) { - RConsume(child, [&](CordRep* child, size_t offset, size_t length) { - if (IsFlatOrExternal(child)) { - rep = PrependLeaf(rep, child, offset, length); + RConsume(child, [&](CordRep* child_arg, size_t offset, size_t len) { + if (IsFlatOrExternal(child_arg)) { + rep = PrependLeaf(rep, child_arg, offset, len); } else { - rep = AddRing<AddMode::kPrepend>(rep, child->ring(), offset, length); + rep = AddRing<AddMode::kPrepend>(rep, child_arg->ring(), offset, len); } }); return Validate(rep); } CordRepRing* CordRepRing::PrependLeaf(CordRepRing* rep, CordRep* child, - size_t offset, size_t length) { + size_t offset, size_t len) { rep = Mutable(rep, 1); index_type head = rep->retreat(rep->head_); pos_type end_pos = rep->begin_pos_; rep->head_ = head; - rep->length += length; - rep->begin_pos_ -= length; + rep->length += len; + rep->begin_pos_ -= len; rep->entry_end_pos()[head] = end_pos; rep->entry_child()[head] = child; rep->entry_data_offset()[head] = static_cast<offset_type>(offset); @@ -787,18 +778,18 @@ char CordRepRing::GetCharacter(size_t offset) const { } CordRepRing* CordRepRing::SubRing(CordRepRing* rep, size_t offset, - size_t length, size_t extra) { + size_t len, size_t extra) { assert(offset <= rep->length); - assert(offset <= rep->length - length); + assert(offset <= rep->length - len); - if (length == 0) { + if (len == 0) { CordRep::Unref(rep); return nullptr; } // Find position of first byte Position head = rep->Find(offset); - Position tail = rep->FindTail(head.index, offset + length); + Position tail = rep->FindTail(head.index, offset + len); const size_t new_entries = rep->entries(head.index, tail.index); if (rep->refcount.IsOne() && extra <= (rep->capacity() - new_entries)) { @@ -815,7 +806,7 @@ CordRepRing* CordRepRing::SubRing(CordRepRing* rep, size_t offset, } // Adjust begin_pos and length - rep->length = length; + rep->length = len; rep->begin_pos_ += offset; // Adjust head and tail blocks @@ -889,10 +880,6 @@ CordRepRing* CordRepRing::RemoveSuffix(CordRepRing* rep, size_t len, return Validate(rep); } -#ifdef __clang__ -#pragma clang diagnostic pop -#endif - } // namespace cord_internal ABSL_NAMESPACE_END } // namespace absl diff --git a/absl/strings/internal/cord_rep_ring.h b/absl/strings/internal/cord_rep_ring.h index 830f2b2a..2082a565 100644 --- a/absl/strings/internal/cord_rep_ring.h +++ b/absl/strings/internal/cord_rep_ring.h @@ -30,15 +30,6 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace cord_internal { -// See https://bugs.llvm.org/show_bug.cgi?id=48477 -#ifdef __clang__ -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wshadow" -#if __has_warning("-Wshadow-field") -#pragma clang diagnostic ignored "-Wshadow-field" -#endif -#endif - // All operations modifying a ring buffer are implemented as static methods // requiring a CordRepRing instance with a reference adopted by the method. // @@ -210,23 +201,23 @@ class CordRepRing : public CordRep { // referencing up to `size` capacity directly before the existing data. Span<char> GetPrependBuffer(size_t size); - // Returns a cord ring buffer containing `length` bytes of data starting at + // Returns a cord ring buffer containing `len` bytes of data starting at // `offset`. If the input is not shared, this function will remove all head // and tail child nodes outside of the requested range, and adjust the new // head and tail nodes as required. If the input is shared, this function // returns a new instance sharing some or all of the nodes from the input. - static CordRepRing* SubRing(CordRepRing* r, size_t offset, size_t length, + static CordRepRing* SubRing(CordRepRing* r, size_t offset, size_t len, size_t extra = 0); - // Returns a cord ring buffer with the first `length` bytes removed. + // Returns a cord ring buffer with the first `len` bytes removed. // If the input is not shared, this function will remove all head child nodes // fully inside the first `length` bytes, and adjust the new head as required. // If the input is shared, this function returns a new instance sharing some // or all of the nodes from the input. - static CordRepRing* RemoveSuffix(CordRepRing* r, size_t length, + static CordRepRing* RemoveSuffix(CordRepRing* r, size_t len, size_t extra = 0); - // Returns a cord ring buffer with the last `length` bytes removed. + // Returns a cord ring buffer with the last `len` bytes removed. // If the input is not shared, this function will remove all head child nodes // fully inside the first `length` bytes, and adjust the new head as required. // If the input is shared, this function returns a new instance sharing some @@ -242,12 +233,12 @@ class CordRepRing : public CordRep { // function returns false, and `fragment` is left unchanged. bool IsFlat(absl::string_view* fragment) const; - // Returns true if the data starting at `offset` with length `length` is + // Returns true if the data starting at `offset` with length `len` is // managed by this instance inside a single contiguous buffer, in which case // the (optional) output parameter `fragment` is set to the contiguous memory // starting at offset `offset` with length `length`. Otherwise, the function // returns false, and `fragment` is left unchanged. - bool IsFlat(size_t offset, size_t length, absl::string_view* fragment) const; + bool IsFlat(size_t offset, size_t len, absl::string_view* fragment) const; // Testing only: set capacity to requested capacity. void SetCapacityForTesting(size_t capacity); @@ -473,10 +464,10 @@ class CordRepRing : public CordRep { size_t length, size_t extra); // Appends or prepends (depending on AddMode) the ring buffer in `ring' to - // `rep` starting at `offset` with length `length`. + // `rep` starting at `offset` with length `len`. template <AddMode mode> static CordRepRing* AddRing(CordRepRing* rep, CordRepRing* ring, - size_t offset, size_t length); + size_t offset, size_t len); // Increases the data offset for entry `index` by `n`. void AddDataOffset(index_type index, size_t n); @@ -596,12 +587,12 @@ inline bool CordRepRing::IsFlat(absl::string_view* fragment) const { return false; } -inline bool CordRepRing::IsFlat(size_t offset, size_t length, +inline bool CordRepRing::IsFlat(size_t offset, size_t len, absl::string_view* fragment) const { const Position pos = Find(offset); const absl::string_view data = entry_data(pos.index); - if (data.length() >= length && data.length() - length >= pos.offset) { - if (fragment) *fragment = data.substr(pos.offset, length); + if (data.length() >= len && data.length() - len >= pos.offset) { + if (fragment) *fragment = data.substr(pos.offset, len); return true; } return false; @@ -609,10 +600,6 @@ inline bool CordRepRing::IsFlat(size_t offset, size_t length, std::ostream& operator<<(std::ostream& s, const CordRepRing& rep); -#ifdef __clang__ -#pragma clang diagnostic pop -#endif - } // namespace cord_internal ABSL_NAMESPACE_END } // namespace absl diff --git a/absl/strings/internal/cordz_functions.cc b/absl/strings/internal/cordz_functions.cc index 6ad864f1..f30080f8 100644 --- a/absl/strings/internal/cordz_functions.cc +++ b/absl/strings/internal/cordz_functions.cc @@ -44,7 +44,10 @@ std::atomic<int> g_cordz_mean_interval(50000); #ifdef ABSL_INTERNAL_CORDZ_ENABLED -ABSL_CONST_INIT thread_local int64_t cordz_next_sample = 0; +// Special negative 'not initialized' per thread value for cordz_next_sample. +static constexpr int64_t kInitCordzNextSample = -1; + +ABSL_CONST_INIT thread_local int64_t cordz_next_sample = kInitCordzNextSample; // kIntervalIfDisabled is the number of profile-eligible events need to occur // before the code will confirm that cordz is still disabled. @@ -77,8 +80,11 @@ ABSL_ATTRIBUTE_NOINLINE bool cordz_should_profile_slow() { } if (cordz_next_sample <= 0) { + // If first check on current thread, check cordz_should_profile() + // again using the created (initial) stride in cordz_next_sample. + const bool initialized = cordz_next_sample != kInitCordzNextSample; cordz_next_sample = exponential_biased_generator.GetStride(mean_interval); - return true; + return initialized || cordz_should_profile(); } --cordz_next_sample; diff --git a/absl/strings/internal/cordz_functions_test.cc b/absl/strings/internal/cordz_functions_test.cc index f2cefae3..350623c1 100644 --- a/absl/strings/internal/cordz_functions_test.cc +++ b/absl/strings/internal/cordz_functions_test.cc @@ -14,6 +14,8 @@ #include "absl/strings/internal/cordz_functions.h" +#include <thread> // NOLINT we need real clean new threads + #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/base/config.h" @@ -63,6 +65,22 @@ TEST(CordzFunctionsTest, ShouldProfileAlways) { set_cordz_mean_interval(orig_sample_rate); } +TEST(CordzFunctionsTest, DoesNotAlwaysSampleFirstCord) { + // Set large enough interval such that the chance of 'tons' of threads + // randomly sampling the first call is infinitely small. + set_cordz_mean_interval(10000); + int tries = 0; + bool sampled = false; + do { + ++tries; + ASSERT_THAT(tries, Le(1000)); + std::thread thread([&sampled] { + sampled = cordz_should_profile(); + }); + thread.join(); + } while (sampled); +} + TEST(CordzFunctionsTest, ShouldProfileRate) { static constexpr int kDesiredMeanInterval = 1000; static constexpr int kSamples = 10000; diff --git a/absl/strings/internal/cordz_info.cc b/absl/strings/internal/cordz_info.cc index a6b045ff..a3a0b9c0 100644 --- a/absl/strings/internal/cordz_info.cc +++ b/absl/strings/internal/cordz_info.cc @@ -293,6 +293,16 @@ void CordzInfo::TrackCord(InlineData& cord, const InlineData& src, cordz_info->Track(); } +void CordzInfo::MaybeTrackCordImpl(InlineData& cord, const InlineData& src, + MethodIdentifier method) { + if (src.is_profiled()) { + TrackCord(cord, src, method); + } else if (cord.is_profiled()) { + cord.cordz_info()->Untrack(); + cord.clear_cordz_info(); + } +} + CordzInfo::MethodIdentifier CordzInfo::GetParentMethod(const CordzInfo* src) { if (src == nullptr) return MethodIdentifier::kUnknown; return src->parent_method_ != MethodIdentifier::kUnknown ? src->parent_method_ diff --git a/absl/strings/internal/cordz_info.h b/absl/strings/internal/cordz_info.h index 29237930..13aaee17 100644 --- a/absl/strings/internal/cordz_info.h +++ b/absl/strings/internal/cordz_info.h @@ -77,13 +77,31 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { static void MaybeTrackCord(InlineData& cord, MethodIdentifier method); // Maybe sample the cord identified by 'cord' for method 'method'. - // `src` identifies a 'parent' cord which content is copied into the current - // cord, typically the input cord for an assign emthod or copy constructor. - // Invokes the corresponding `TrackCord` method if either cord is sampled, or - // if `cord` is randomly picked for sampling. Possible scenarios: - // * `src` is sampled: `cord` will be set to sampled if not already sampled. - // Parent stack and update stats of `src` are copied into `cord` - // * `src` is not sampled: `cord` may be randomly picked for sampling. + // `src` identifies a 'parent' cord which is assigned to `cord`, typically the + // input cord for a copy constructor, or an assign method such as `operator=` + // `cord` will be sampled if (and only if) `src` is sampled. + // If `cord` is currently being sampled and `src` is not being sampled, then + // this function will stop sampling the cord and reset the cord's cordz_info. + // + // Previously this function defined that `cord` will be sampled if either + // `src` is sampled, or if `cord` is randomly picked for sampling. However, + // this can cause issues, as there may be paths where some cord is assigned an + // indirect copy of it's own value. As such a 'string of copies' would then + // remain sampled (`src.is_profiled`), then assigning such a cord back to + // 'itself' creates a cycle where the cord will converge to 'always sampled`. + // + // For example: + // + // Cord x; + // for (...) { + // // Copy ctor --> y.is_profiled := x.is_profiled | random(...) + // Cord y = x; + // ... + // // Assign x = y --> x.is_profiled = y.is_profiled | random(...) + // // ==> x.is_profiled |= random(...) + // // ==> x converges to 'always profiled' + // x = y; + // } static void MaybeTrackCord(InlineData& cord, const InlineData& src, MethodIdentifier method); @@ -201,6 +219,12 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { #endif } + // Non-inlined implementation of `MaybeTrackCord`, which is executed if + // either `src` is sampled or `cord` is sampled, and either untracks or + // tracks `cord` as documented per `MaybeTrackCord`. + static void MaybeTrackCordImpl(InlineData& cord, const InlineData& src, + MethodIdentifier method); + ABSL_CONST_INIT static List global_list_; List* const list_ = &global_list_; @@ -232,9 +256,8 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void CordzInfo::MaybeTrackCord( inline ABSL_ATTRIBUTE_ALWAYS_INLINE void CordzInfo::MaybeTrackCord( InlineData& cord, const InlineData& src, MethodIdentifier method) { - if (ABSL_PREDICT_FALSE(InlineData::is_either_profiled(cord, src)) || - ABSL_PREDICT_FALSE(cordz_should_profile())) { - TrackCord(cord, src, method); + if (ABSL_PREDICT_FALSE(InlineData::is_either_profiled(cord, src))) { + MaybeTrackCordImpl(cord, src, method); } } diff --git a/absl/strings/internal/cordz_info_test.cc b/absl/strings/internal/cordz_info_test.cc index 59a8c525..b98343ae 100644 --- a/absl/strings/internal/cordz_info_test.cc +++ b/absl/strings/internal/cordz_info_test.cc @@ -74,19 +74,49 @@ TEST(CordzInfoTest, TrackCord) { info->Untrack(); } -TEST(CordzInfoTest, MaybeTrackCordOnSampledCord) { - TestCordData data1; - CordzInfo::TrackCord(data1.data, kTrackCordMethod); - CordzInfo* info1 = data1.data.cordz_info(); - TestCordData data2; - CordzInfo::MaybeTrackCord(data2.data, data1.data, kTrackCordMethod); - CordzInfo* info2 = data2.data.cordz_info(); - ASSERT_THAT(info2, Ne(nullptr)); - EXPECT_THAT(info2->GetCordRepForTesting(), Eq(data2.rep.rep)); - info2->Untrack(); - info1->Untrack(); +TEST(CordzInfoTest, MaybeTrackChildCordWithoutSampling) { + CordzSamplingIntervalHelper sample_none(99999); + TestCordData parent, child; + CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod); + EXPECT_THAT(child.data.cordz_info(), Eq(nullptr)); +} + +TEST(CordzInfoTest, MaybeTrackChildCordWithSampling) { + CordzSamplingIntervalHelper sample_all(1); + TestCordData parent, child; + CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod); + EXPECT_THAT(child.data.cordz_info(), Eq(nullptr)); } +TEST(CordzInfoTest, MaybeTrackChildCordWithoutSamplingParentSampled) { + CordzSamplingIntervalHelper sample_none(99999); + TestCordData parent, child; + CordzInfo::TrackCord(parent.data, kTrackCordMethod); + CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod); + CordzInfo* parent_info = parent.data.cordz_info(); + CordzInfo* child_info = child.data.cordz_info(); + ASSERT_THAT(child_info, Ne(nullptr)); + EXPECT_THAT(child_info->GetCordRepForTesting(), Eq(child.rep.rep)); + EXPECT_THAT(child_info->GetParentStack(), parent_info->GetStack()); + parent_info->Untrack(); + child_info->Untrack(); +} + +TEST(CordzInfoTest, MaybeTrackChildCordWithoutSamplingChildSampled) { + CordzSamplingIntervalHelper sample_none(99999); + TestCordData parent, child; + CordzInfo::TrackCord(child.data, kTrackCordMethod); + CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod); + EXPECT_THAT(child.data.cordz_info(), Eq(nullptr)); +} + +TEST(CordzInfoTest, MaybeTrackChildCordWithSamplingChildSampled) { + CordzSamplingIntervalHelper sample_all(1); + TestCordData parent, child; + CordzInfo::TrackCord(child.data, kTrackCordMethod); + CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod); + EXPECT_THAT(child.data.cordz_info(), Eq(nullptr)); +} TEST(CordzInfoTest, UntrackCord) { TestCordData data; diff --git a/absl/strings/internal/str_format/bind.cc b/absl/strings/internal/str_format/bind.cc index 4e68b90b..c988ba8f 100644 --- a/absl/strings/internal/str_format/bind.cc +++ b/absl/strings/internal/str_format/bind.cc @@ -58,7 +58,7 @@ inline bool ArgContext::Bind(const UnboundConversion* unbound, if (static_cast<size_t>(arg_position - 1) >= pack_.size()) return false; arg = &pack_[arg_position - 1]; // 1-based - if (!unbound->flags.basic) { + if (unbound->flags != Flags::kBasic) { int width = unbound->width.value(); bool force_left = false; if (unbound->width.is_from_arg()) { @@ -84,9 +84,8 @@ inline bool ArgContext::Bind(const UnboundConversion* unbound, FormatConversionSpecImplFriend::SetPrecision(precision, bound); if (force_left) { - Flags flags = unbound->flags; - flags.left = true; - FormatConversionSpecImplFriend::SetFlags(flags, bound); + FormatConversionSpecImplFriend::SetFlags(unbound->flags | Flags::kLeft, + bound); } else { FormatConversionSpecImplFriend::SetFlags(unbound->flags, bound); } diff --git a/absl/strings/internal/str_format/extension.cc b/absl/strings/internal/str_format/extension.cc index bb0d96cf..484f6ebf 100644 --- a/absl/strings/internal/str_format/extension.cc +++ b/absl/strings/internal/str_format/extension.cc @@ -23,13 +23,13 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace str_format_internal { -std::string Flags::ToString() const { +std::string FlagsToString(Flags v) { std::string s; - s.append(left ? "-" : ""); - s.append(show_pos ? "+" : ""); - s.append(sign_col ? " " : ""); - s.append(alt ? "#" : ""); - s.append(zero ? "0" : ""); + s.append(FlagsContains(v, Flags::kLeft) ? "-" : ""); + s.append(FlagsContains(v, Flags::kShowPos) ? "+" : ""); + s.append(FlagsContains(v, Flags::kSignCol) ? " " : ""); + s.append(FlagsContains(v, Flags::kAlt) ? "#" : ""); + s.append(FlagsContains(v, Flags::kZero) ? "0" : ""); return s; } diff --git a/absl/strings/internal/str_format/extension.h b/absl/strings/internal/str_format/extension.h index a9b9e137..55cbb56d 100644 --- a/absl/strings/internal/str_format/extension.h +++ b/absl/strings/internal/str_format/extension.h @@ -128,19 +128,33 @@ class FormatSinkImpl { char buf_[1024]; }; -struct Flags { - bool basic : 1; // fastest conversion: no flags, width, or precision - bool left : 1; // "-" - bool show_pos : 1; // "+" - bool sign_col : 1; // " " - bool alt : 1; // "#" - bool zero : 1; // "0" - std::string ToString() const; - friend std::ostream& operator<<(std::ostream& os, const Flags& v) { - return os << v.ToString(); - } +enum class Flags : uint8_t { + kBasic = 0, + kLeft = 1 << 0, + kShowPos = 1 << 1, + kSignCol = 1 << 2, + kAlt = 1 << 3, + kZero = 1 << 4, + // This is not a real flag. It just exists to turn off kBasic when no other + // flags are set. This is for when width/precision are specified. + kNonBasic = 1 << 5, }; +constexpr Flags operator|(Flags a, Flags b) { + return static_cast<Flags>(static_cast<uint8_t>(a) | static_cast<uint8_t>(b)); +} + +constexpr bool FlagsContains(Flags haystack, Flags needle) { + return (static_cast<uint8_t>(haystack) & static_cast<uint8_t>(needle)) == + static_cast<uint8_t>(needle); +} + +std::string FlagsToString(Flags v); + +inline std::ostream& operator<<(std::ostream& os, Flags v) { + return os << FlagsToString(v); +} + // clang-format off #define ABSL_INTERNAL_CONVERSION_CHARS_EXPAND_(X_VAL, X_SEP) \ /* text */ \ @@ -257,12 +271,16 @@ struct FormatConversionSpecImplFriend; class FormatConversionSpecImpl { public: // Width and precison are not specified, no flags are set. - bool is_basic() const { return flags_.basic; } - bool has_left_flag() const { return flags_.left; } - bool has_show_pos_flag() const { return flags_.show_pos; } - bool has_sign_col_flag() const { return flags_.sign_col; } - bool has_alt_flag() const { return flags_.alt; } - bool has_zero_flag() const { return flags_.zero; } + bool is_basic() const { return flags_ == Flags::kBasic; } + bool has_left_flag() const { return FlagsContains(flags_, Flags::kLeft); } + bool has_show_pos_flag() const { + return FlagsContains(flags_, Flags::kShowPos); + } + bool has_sign_col_flag() const { + return FlagsContains(flags_, Flags::kSignCol); + } + bool has_alt_flag() const { return FlagsContains(flags_, Flags::kAlt); } + bool has_zero_flag() const { return FlagsContains(flags_, Flags::kZero); } FormatConversionChar conversion_char() const { // Keep this field first in the struct . It generates better code when @@ -306,7 +324,7 @@ struct FormatConversionSpecImplFriend final { conv->precision_ = p; } static std::string FlagsToString(const FormatConversionSpecImpl& spec) { - return spec.flags_.ToString(); + return str_format_internal::FlagsToString(spec.flags_); } }; diff --git a/absl/strings/internal/str_format/parser.cc b/absl/strings/internal/str_format/parser.cc index f308d023..2c9c07da 100644 --- a/absl/strings/internal/str_format/parser.cc +++ b/absl/strings/internal/str_format/parser.cc @@ -34,60 +34,67 @@ namespace str_format_internal { using CC = FormatConversionCharInternal; using LM = LengthMod; +// Abbreviations to fit in the table below. +constexpr auto f_sign = Flags::kSignCol; +constexpr auto f_alt = Flags::kAlt; +constexpr auto f_pos = Flags::kShowPos; +constexpr auto f_left = Flags::kLeft; +constexpr auto f_zero = Flags::kZero; + ABSL_CONST_INIT const ConvTag kTags[256] = { - {}, {}, {}, {}, {}, {}, {}, {}, // 00-07 - {}, {}, {}, {}, {}, {}, {}, {}, // 08-0f - {}, {}, {}, {}, {}, {}, {}, {}, // 10-17 - {}, {}, {}, {}, {}, {}, {}, {}, // 18-1f - {}, {}, {}, {}, {}, {}, {}, {}, // 20-27 - {}, {}, {}, {}, {}, {}, {}, {}, // 28-2f - {}, {}, {}, {}, {}, {}, {}, {}, // 30-37 - {}, {}, {}, {}, {}, {}, {}, {}, // 38-3f - {}, CC::A, {}, {}, {}, CC::E, CC::F, CC::G, // @ABCDEFG - {}, {}, {}, {}, LM::L, {}, {}, {}, // HIJKLMNO - {}, {}, {}, {}, {}, {}, {}, {}, // PQRSTUVW - CC::X, {}, {}, {}, {}, {}, {}, {}, // XYZ[\]^_ - {}, CC::a, {}, CC::c, CC::d, CC::e, CC::f, CC::g, // `abcdefg - LM::h, CC::i, LM::j, {}, LM::l, {}, CC::n, CC::o, // hijklmno - CC::p, LM::q, {}, CC::s, LM::t, CC::u, {}, {}, // pqrstuvw - CC::x, {}, LM::z, {}, {}, {}, {}, {}, // xyz{|}! - {}, {}, {}, {}, {}, {}, {}, {}, // 80-87 - {}, {}, {}, {}, {}, {}, {}, {}, // 88-8f - {}, {}, {}, {}, {}, {}, {}, {}, // 90-97 - {}, {}, {}, {}, {}, {}, {}, {}, // 98-9f - {}, {}, {}, {}, {}, {}, {}, {}, // a0-a7 - {}, {}, {}, {}, {}, {}, {}, {}, // a8-af - {}, {}, {}, {}, {}, {}, {}, {}, // b0-b7 - {}, {}, {}, {}, {}, {}, {}, {}, // b8-bf - {}, {}, {}, {}, {}, {}, {}, {}, // c0-c7 - {}, {}, {}, {}, {}, {}, {}, {}, // c8-cf - {}, {}, {}, {}, {}, {}, {}, {}, // d0-d7 - {}, {}, {}, {}, {}, {}, {}, {}, // d8-df - {}, {}, {}, {}, {}, {}, {}, {}, // e0-e7 - {}, {}, {}, {}, {}, {}, {}, {}, // e8-ef - {}, {}, {}, {}, {}, {}, {}, {}, // f0-f7 - {}, {}, {}, {}, {}, {}, {}, {}, // f8-ff + {}, {}, {}, {}, {}, {}, {}, {}, // 00-07 + {}, {}, {}, {}, {}, {}, {}, {}, // 08-0f + {}, {}, {}, {}, {}, {}, {}, {}, // 10-17 + {}, {}, {}, {}, {}, {}, {}, {}, // 18-1f + f_sign, {}, {}, f_alt, {}, {}, {}, {}, // !"#$%&' + {}, {}, {}, f_pos, {}, f_left, {}, {}, // ()*+,-./ + f_zero, {}, {}, {}, {}, {}, {}, {}, // 01234567 + {}, {}, {}, {}, {}, {}, {}, {}, // 89:;<=>? + {}, CC::A, {}, {}, {}, CC::E, CC::F, CC::G, // @ABCDEFG + {}, {}, {}, {}, LM::L, {}, {}, {}, // HIJKLMNO + {}, {}, {}, {}, {}, {}, {}, {}, // PQRSTUVW + CC::X, {}, {}, {}, {}, {}, {}, {}, // XYZ[\]^_ + {}, CC::a, {}, CC::c, CC::d, CC::e, CC::f, CC::g, // `abcdefg + LM::h, CC::i, LM::j, {}, LM::l, {}, CC::n, CC::o, // hijklmno + CC::p, LM::q, {}, CC::s, LM::t, CC::u, {}, {}, // pqrstuvw + CC::x, {}, LM::z, {}, {}, {}, {}, {}, // xyz{|}! + {}, {}, {}, {}, {}, {}, {}, {}, // 80-87 + {}, {}, {}, {}, {}, {}, {}, {}, // 88-8f + {}, {}, {}, {}, {}, {}, {}, {}, // 90-97 + {}, {}, {}, {}, {}, {}, {}, {}, // 98-9f + {}, {}, {}, {}, {}, {}, {}, {}, // a0-a7 + {}, {}, {}, {}, {}, {}, {}, {}, // a8-af + {}, {}, {}, {}, {}, {}, {}, {}, // b0-b7 + {}, {}, {}, {}, {}, {}, {}, {}, // b8-bf + {}, {}, {}, {}, {}, {}, {}, {}, // c0-c7 + {}, {}, {}, {}, {}, {}, {}, {}, // c8-cf + {}, {}, {}, {}, {}, {}, {}, {}, // d0-d7 + {}, {}, {}, {}, {}, {}, {}, {}, // d8-df + {}, {}, {}, {}, {}, {}, {}, {}, // e0-e7 + {}, {}, {}, {}, {}, {}, {}, {}, // e8-ef + {}, {}, {}, {}, {}, {}, {}, {}, // f0-f7 + {}, {}, {}, {}, {}, {}, {}, {}, // f8-ff }; namespace { bool CheckFastPathSetting(const UnboundConversion& conv) { - bool should_be_basic = !conv.flags.left && // - !conv.flags.show_pos && // - !conv.flags.sign_col && // - !conv.flags.alt && // - !conv.flags.zero && // - (conv.width.value() == -1) && - (conv.precision.value() == -1); - if (should_be_basic != conv.flags.basic) { + bool width_precision_needed = + conv.width.value() >= 0 || conv.precision.value() >= 0; + if (width_precision_needed && conv.flags == Flags::kBasic) { fprintf(stderr, "basic=%d left=%d show_pos=%d sign_col=%d alt=%d zero=%d " "width=%d precision=%d\n", - conv.flags.basic, conv.flags.left, conv.flags.show_pos, - conv.flags.sign_col, conv.flags.alt, conv.flags.zero, - conv.width.value(), conv.precision.value()); + conv.flags == Flags::kBasic ? 1 : 0, + FlagsContains(conv.flags, Flags::kLeft) ? 1 : 0, + FlagsContains(conv.flags, Flags::kShowPos) ? 1 : 0, + FlagsContains(conv.flags, Flags::kSignCol) ? 1 : 0, + FlagsContains(conv.flags, Flags::kAlt) ? 1 : 0, + FlagsContains(conv.flags, Flags::kZero) ? 1 : 0, conv.width.value(), + conv.precision.value()); + return false; } - return should_be_basic == conv.flags.basic; + return true; } template <bool is_positional> @@ -131,40 +138,21 @@ const char *ConsumeConversion(const char *pos, const char *const end, ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR(); // We should start with the basic flag on. - assert(conv->flags.basic); + assert(conv->flags == Flags::kBasic); // Any non alpha character makes this conversion not basic. // This includes flags (-+ #0), width (1-9, *) or precision (.). // All conversion characters and length modifiers are alpha characters. if (c < 'A') { - conv->flags.basic = false; - - for (; c <= '0';) { - // FIXME: We might be able to speed this up reusing the lookup table from - // above. It might require changing Flags to be a plain integer where we - // can |= a value. - switch (c) { - case '-': - conv->flags.left = true; - break; - case '+': - conv->flags.show_pos = true; - break; - case ' ': - conv->flags.sign_col = true; - break; - case '#': - conv->flags.alt = true; - break; - case '0': - conv->flags.zero = true; - break; - default: - goto flags_done; + while (c <= '0') { + auto tag = GetTagForChar(c); + if (tag.is_flags()) { + conv->flags = conv->flags | tag.as_flags(); + ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR(); + } else { + break; } - ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR(); } -flags_done: if (c <= '9') { if (c >= '0') { @@ -173,12 +161,12 @@ flags_done: if (ABSL_PREDICT_FALSE(*next_arg != 0)) return nullptr; // Positional conversion. *next_arg = -1; - conv->flags = Flags(); - conv->flags.basic = true; return ConsumeConversion<true>(original_pos, end, conv, next_arg); } + conv->flags = conv->flags | Flags::kNonBasic; conv->width.set_value(maybe_width); } else if (c == '*') { + conv->flags = conv->flags | Flags::kNonBasic; ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR(); if (is_positional) { if (ABSL_PREDICT_FALSE(c < '1' || c > '9')) return nullptr; @@ -192,6 +180,7 @@ flags_done: } if (c == '.') { + conv->flags = conv->flags | Flags::kNonBasic; ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR(); if (std::isdigit(c)) { conv->precision.set_value(parse_digits()); diff --git a/absl/strings/internal/str_format/parser.h b/absl/strings/internal/str_format/parser.h index 6504dd3d..ad8646ed 100644 --- a/absl/strings/internal/str_format/parser.h +++ b/absl/strings/internal/str_format/parser.h @@ -41,10 +41,7 @@ std::string LengthModToString(LengthMod v); // The analyzed properties of a single specified conversion. struct UnboundConversion { - UnboundConversion() - : flags() /* This is required to zero all the fields of flags. */ { - flags.basic = true; - } + UnboundConversion() {} class InputValue { public: @@ -79,7 +76,7 @@ struct UnboundConversion { InputValue width; InputValue precision; - Flags flags; + Flags flags = Flags::kBasic; LengthMod length_mod = LengthMod::none; FormatConversionChar conv = FormatConversionCharInternal::kNone; }; @@ -93,32 +90,43 @@ const char* ConsumeUnboundConversion(const char* p, const char* end, UnboundConversion* conv, int* next_arg); // Helper tag class for the table below. -// It allows fast `char -> ConversionChar/LengthMod` checking and +// It allows fast `char -> ConversionChar/LengthMod/Flags` checking and // conversions. class ConvTag { public: constexpr ConvTag(FormatConversionChar conversion_char) // NOLINT - : tag_(static_cast<int8_t>(conversion_char)) {} - // We invert the length modifiers to make them negative so that we can easily - // test for them. + : tag_(static_cast<uint8_t>(conversion_char)) {} constexpr ConvTag(LengthMod length_mod) // NOLINT - : tag_(~static_cast<std::int8_t>(length_mod)) {} - // Everything else is -128, which is negative to make is_conv() simpler. - constexpr ConvTag() : tag_(-128) {} + : tag_(0x80 | static_cast<uint8_t>(length_mod)) {} + constexpr ConvTag(Flags flags) // NOLINT + : tag_(0xc0 | static_cast<uint8_t>(flags)) {} + constexpr ConvTag() : tag_(0xFF) {} + + bool is_conv() const { return (tag_ & 0x80) == 0; } + bool is_length() const { return (tag_ & 0xC0) == 0x80; } + bool is_flags() const { return (tag_ & 0xE0) == 0xC0; } - bool is_conv() const { return tag_ >= 0; } - bool is_length() const { return tag_ < 0 && tag_ != -128; } FormatConversionChar as_conv() const { assert(is_conv()); + assert(!is_length()); + assert(!is_flags()); return static_cast<FormatConversionChar>(tag_); } LengthMod as_length() const { + assert(!is_conv()); assert(is_length()); - return static_cast<LengthMod>(~tag_); + assert(!is_flags()); + return static_cast<LengthMod>(tag_ & 0x3F); + } + Flags as_flags() const { + assert(!is_conv()); + assert(!is_length()); + assert(is_flags()); + return static_cast<Flags>(tag_ & 0x1F); } private: - std::int8_t tag_; + uint8_t tag_; }; extern const ConvTag kTags[256]; diff --git a/absl/strings/internal/str_format/parser_test.cc b/absl/strings/internal/str_format/parser_test.cc index a5fa1c79..fe0d2963 100644 --- a/absl/strings/internal/str_format/parser_test.cc +++ b/absl/strings/internal/str_format/parser_test.cc @@ -270,15 +270,22 @@ TEST_F(ConsumeUnboundConversionTest, Flags) { for (int k = 0; k < kNumFlags; ++k) if ((i >> k) & 1) fmt += kAllFlags[k]; // flag order shouldn't matter - if (rev == 1) { std::reverse(fmt.begin(), fmt.end()); } + if (rev == 1) { + std::reverse(fmt.begin(), fmt.end()); + } fmt += 'd'; SCOPED_TRACE(fmt); EXPECT_TRUE(Run(fmt.c_str())); - EXPECT_EQ(fmt.find('-') == std::string::npos, !o.flags.left); - EXPECT_EQ(fmt.find('+') == std::string::npos, !o.flags.show_pos); - EXPECT_EQ(fmt.find(' ') == std::string::npos, !o.flags.sign_col); - EXPECT_EQ(fmt.find('#') == std::string::npos, !o.flags.alt); - EXPECT_EQ(fmt.find('0') == std::string::npos, !o.flags.zero); + EXPECT_EQ(fmt.find('-') == std::string::npos, + !FlagsContains(o.flags, Flags::kLeft)); + EXPECT_EQ(fmt.find('+') == std::string::npos, + !FlagsContains(o.flags, Flags::kShowPos)); + EXPECT_EQ(fmt.find(' ') == std::string::npos, + !FlagsContains(o.flags, Flags::kSignCol)); + EXPECT_EQ(fmt.find('#') == std::string::npos, + !FlagsContains(o.flags, Flags::kAlt)); + EXPECT_EQ(fmt.find('0') == std::string::npos, + !FlagsContains(o.flags, Flags::kZero)); } } } @@ -288,14 +295,14 @@ TEST_F(ConsumeUnboundConversionTest, BasicFlag) { for (const char* fmt : {"d", "llx", "G", "1$X"}) { SCOPED_TRACE(fmt); EXPECT_TRUE(Run(fmt)); - EXPECT_TRUE(o.flags.basic); + EXPECT_EQ(o.flags, Flags::kBasic); } // Flag is off for (const char* fmt : {"3d", ".llx", "-G", "1$#X"}) { SCOPED_TRACE(fmt); EXPECT_TRUE(Run(fmt)); - EXPECT_FALSE(o.flags.basic); + EXPECT_NE(o.flags, Flags::kBasic); } } |