Commit b4e1b068 authored by Patrick Monette's avatar Patrick Monette Committed by Chromium LUCI CQ

[PM] Get rid of kInvalidVoterId

The IdType class already has built-in support for an invalid ID.

Bug: 971272
Change-Id: I7121400d72c22b90c153da32f0a0f74e46a27569
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545442
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833369}
parent 5810031b
...@@ -248,7 +248,7 @@ BoostingVoteAggregator::~BoostingVoteAggregator() { ...@@ -248,7 +248,7 @@ BoostingVoteAggregator::~BoostingVoteAggregator() {
VotingChannel BoostingVoteAggregator::GetVotingChannel() { VotingChannel BoostingVoteAggregator::GetVotingChannel() {
DCHECK(nodes_.empty()); DCHECK(nodes_.empty());
DCHECK_EQ(voting::kInvalidVoterId<Vote>, input_voter_id_); DCHECK(!input_voter_id_);
DCHECK_GT(1u, vote_consumer_default_impl_.voting_channels_issued()); DCHECK_GT(1u, vote_consumer_default_impl_.voting_channels_issued());
auto channel = vote_consumer_default_impl_.BuildVotingChannel(); auto channel = vote_consumer_default_impl_.BuildVotingChannel();
input_voter_id_ = channel.voter_id(); input_voter_id_ = channel.voter_id();
...@@ -260,7 +260,7 @@ void BoostingVoteAggregator::SetUpstreamVotingChannel(VotingChannel&& channel) { ...@@ -260,7 +260,7 @@ void BoostingVoteAggregator::SetUpstreamVotingChannel(VotingChannel&& channel) {
} }
bool BoostingVoteAggregator::IsSetup() const { bool BoostingVoteAggregator::IsSetup() const {
return input_voter_id_ != voting::kInvalidVoterId<Vote> && channel_.IsValid(); return input_voter_id_ && channel_.IsValid();
} }
void BoostingVoteAggregator::SubmitBoostingVote( void BoostingVoteAggregator::SubmitBoostingVote(
......
...@@ -367,7 +367,7 @@ class BoostingVoteAggregator : public VoteObserver { ...@@ -367,7 +367,7 @@ class BoostingVoteAggregator : public VoteObserver {
// Our input voter. We'll only accept votes from this voter otherwise we'll // Our input voter. We'll only accept votes from this voter otherwise we'll
// DCHECK. // DCHECK.
voting::VoterId<Vote> input_voter_id_ = voting::kInvalidVoterId<Vote>; voting::VoterId<Vote> input_voter_id_;
// Our channel for upstreaming our votes. // Our channel for upstreaming our votes.
VotingChannelWrapper channel_; VotingChannelWrapper channel_;
......
...@@ -129,7 +129,7 @@ TEST(ExecutionContextPriorityTest, VoteReceiptsWork) { ...@@ -129,7 +129,7 @@ TEST(ExecutionContextPriorityTest, VoteReceiptsWork) {
voter.SetVotingChannel(consumer.voting_channel_factory_.BuildVotingChannel()); voter.SetVotingChannel(consumer.voting_channel_factory_.BuildVotingChannel());
EXPECT_EQ(&consumer.voting_channel_factory_, EXPECT_EQ(&consumer.voting_channel_factory_,
voter.voting_channel_.factory_for_testing()); voter.voting_channel_.factory_for_testing());
EXPECT_NE(voting::kInvalidVoterId<Vote>, voter.voting_channel_.voter_id()); EXPECT_TRUE(voter.voting_channel_.voter_id());
EXPECT_TRUE(voter.voting_channel_.IsValid()); EXPECT_TRUE(voter.voting_channel_.IsValid());
voter.EmitVote(kDummyExecutionContext1, base::TaskPriority::LOWEST); voter.EmitVote(kDummyExecutionContext1, base::TaskPriority::LOWEST);
......
...@@ -14,7 +14,7 @@ OverrideVoteAggregator::~OverrideVoteAggregator() = default; ...@@ -14,7 +14,7 @@ OverrideVoteAggregator::~OverrideVoteAggregator() = default;
VotingChannel OverrideVoteAggregator::GetOverrideVotingChannel() { VotingChannel OverrideVoteAggregator::GetOverrideVotingChannel() {
DCHECK(vote_data_map_.empty()); DCHECK(vote_data_map_.empty());
DCHECK_EQ(voting::kInvalidVoterId<Vote>, override_voter_id_); DCHECK(!override_voter_id_);
DCHECK_GT(2u, vote_consumer_default_impl_.voting_channels_issued()); DCHECK_GT(2u, vote_consumer_default_impl_.voting_channels_issued());
auto channel = vote_consumer_default_impl_.BuildVotingChannel(); auto channel = vote_consumer_default_impl_.BuildVotingChannel();
override_voter_id_ = channel.voter_id(); override_voter_id_ = channel.voter_id();
...@@ -23,7 +23,7 @@ VotingChannel OverrideVoteAggregator::GetOverrideVotingChannel() { ...@@ -23,7 +23,7 @@ VotingChannel OverrideVoteAggregator::GetOverrideVotingChannel() {
VotingChannel OverrideVoteAggregator::GetDefaultVotingChannel() { VotingChannel OverrideVoteAggregator::GetDefaultVotingChannel() {
DCHECK(vote_data_map_.empty()); DCHECK(vote_data_map_.empty());
DCHECK_EQ(voting::kInvalidVoterId<Vote>, default_voter_id_); DCHECK(!default_voter_id_);
DCHECK_GT(2u, vote_consumer_default_impl_.voting_channels_issued()); DCHECK_GT(2u, vote_consumer_default_impl_.voting_channels_issued());
auto channel = vote_consumer_default_impl_.BuildVotingChannel(); auto channel = vote_consumer_default_impl_.BuildVotingChannel();
default_voter_id_ = channel.voter_id(); default_voter_id_ = channel.voter_id();
...@@ -35,9 +35,7 @@ void OverrideVoteAggregator::SetUpstreamVotingChannel(VotingChannel&& channel) { ...@@ -35,9 +35,7 @@ void OverrideVoteAggregator::SetUpstreamVotingChannel(VotingChannel&& channel) {
} }
bool OverrideVoteAggregator::IsSetup() const { bool OverrideVoteAggregator::IsSetup() const {
return override_voter_id_ != voting::kInvalidVoterId<Vote> && return override_voter_id_ && default_voter_id_ && channel_.IsValid();
default_voter_id_ != voting::kInvalidVoterId<Vote> &&
channel_.IsValid();
} }
void OverrideVoteAggregator::OnVoteSubmitted( void OverrideVoteAggregator::OnVoteSubmitted(
......
...@@ -87,8 +87,8 @@ class OverrideVoteAggregator : public VoteObserver { ...@@ -87,8 +87,8 @@ class OverrideVoteAggregator : public VoteObserver {
// Our two input voters. We'll only accept votes from these voters otherwise // Our two input voters. We'll only accept votes from these voters otherwise
// we'll DCHECK. // we'll DCHECK.
VoterId override_voter_id_ = voting::kInvalidVoterId<Vote>; VoterId override_voter_id_;
VoterId default_voter_id_ = voting::kInvalidVoterId<Vote>; VoterId default_voter_id_;
// Our channel for upstreaming our votes. // Our channel for upstreaming our votes.
VotingChannelWrapper channel_; VotingChannelWrapper channel_;
......
...@@ -62,6 +62,7 @@ void RootVoteObserver::OnVoteChanged(VoterId voter_id, ...@@ -62,6 +62,7 @@ void RootVoteObserver::OnVoteChanged(VoterId voter_id,
void RootVoteObserver::OnVoteInvalidated( void RootVoteObserver::OnVoteInvalidated(
VoterId voter_id, VoterId voter_id,
const ExecutionContext* execution_context) { const ExecutionContext* execution_context) {
DCHECK_EQ(voter_id_, voter_id);
SetPriorityAndReason( SetPriorityAndReason(
execution_context, execution_context,
PriorityAndReason(base::TaskPriority::LOWEST, PriorityAndReason(base::TaskPriority::LOWEST,
......
...@@ -39,7 +39,7 @@ class RootVoteObserver : public VoteObserver { ...@@ -39,7 +39,7 @@ class RootVoteObserver : public VoteObserver {
VoteConsumerDefaultImpl vote_consumer_default_impl_; VoteConsumerDefaultImpl vote_consumer_default_impl_;
// The ID of the only voting channel we've vended. // The ID of the only voting channel we've vended.
voting::VoterId<Vote> voter_id_ = voting::kInvalidVoterId<Vote>; VoterId voter_id_;
}; };
} // namespace execution_context_priority } // namespace execution_context_priority
......
...@@ -106,8 +106,6 @@ class Vote final { ...@@ -106,8 +106,6 @@ class Vote final {
template <typename VoteImpl> template <typename VoteImpl>
using VoterId = util::IdTypeU32<VoteImpl>; using VoterId = util::IdTypeU32<VoteImpl>;
template <typename VoteImpl>
constexpr VoterId<VoteImpl> kInvalidVoterId;
// A raw vote becomes an AcceptedVote once a VoteConsumer receives and stores // A raw vote becomes an AcceptedVote once a VoteConsumer receives and stores
// it, associating it with a VoterId. // it, associating it with a VoterId.
...@@ -252,7 +250,7 @@ class AcceptedVote final { ...@@ -252,7 +250,7 @@ class AcceptedVote final {
// The ID of the voter that submitted the vote. This is defined by the // The ID of the voter that submitted the vote. This is defined by the
// VoteConsumer. // VoteConsumer.
VoterId<VoteImpl> voter_id_ = kInvalidVoterId<VoteImpl>; VoterId<VoteImpl> voter_id_;
const ContextType* context_ = nullptr; const ContextType* context_ = nullptr;
...@@ -313,7 +311,7 @@ class VotingChannel final { ...@@ -313,7 +311,7 @@ class VotingChannel final {
// Used to reach back into the factory to decrement the outstanding // Used to reach back into the factory to decrement the outstanding
// VotingChannel count, and for routing votes to the consumer. // VotingChannel count, and for routing votes to the consumer.
VotingChannelFactory<VoteImpl>* factory_ = nullptr; VotingChannelFactory<VoteImpl>* factory_ = nullptr;
VoterId<VoteImpl> voter_id_ = kInvalidVoterId<VoteImpl>; VoterId<VoteImpl> voter_id_;
}; };
// A helper for creating VotingChannels that binds a unique VoterId (and // A helper for creating VotingChannels that binds a unique VoterId (and
...@@ -662,7 +660,7 @@ AcceptedVote<VoteImpl>::AcceptedVote(VoteConsumer<VoteImpl>* consumer, ...@@ -662,7 +660,7 @@ AcceptedVote<VoteImpl>::AcceptedVote(VoteConsumer<VoteImpl>* consumer,
vote_(vote), vote_(vote),
invalidated_(false) { invalidated_(false) {
DCHECK(consumer_); DCHECK(consumer_);
DCHECK_NE(voter_id_, kInvalidVoterId<VoteImpl>); DCHECK(voter_id_);
DCHECK(context_); DCHECK(context_);
DCHECK(vote_.IsValid()); DCHECK(vote_.IsValid());
} }
...@@ -700,8 +698,7 @@ bool AcceptedVote<VoteImpl>::HasReceipt( ...@@ -700,8 +698,7 @@ bool AcceptedVote<VoteImpl>::HasReceipt(
template <class VoteImpl> template <class VoteImpl>
bool AcceptedVote<VoteImpl>::IsValid() const { bool AcceptedVote<VoteImpl>::IsValid() const {
return consumer_ && voter_id_ != kInvalidVoterId<VoteImpl> && return consumer_ && voter_id_ && vote_.IsValid() && !invalidated_;
vote_.IsValid() && !invalidated_;
} }
template <class VoteImpl> template <class VoteImpl>
...@@ -776,7 +773,7 @@ void AcceptedVote<VoteImpl>::Take(AcceptedVote<VoteImpl>&& rhs) { ...@@ -776,7 +773,7 @@ void AcceptedVote<VoteImpl>::Take(AcceptedVote<VoteImpl>&& rhs) {
DCHECK(!receipt_); DCHECK(!receipt_);
consumer_ = std::exchange(rhs.consumer_, nullptr); consumer_ = std::exchange(rhs.consumer_, nullptr);
voter_id_ = std::exchange(rhs.voter_id_, kInvalidVoterId<VoteImpl>); voter_id_ = std::exchange(rhs.voter_id_, VoterId<VoteImpl>());
context_ = std::exchange(rhs.context_, nullptr); context_ = std::exchange(rhs.context_, nullptr);
vote_ = std::exchange(rhs.vote_, VoteImpl()); vote_ = std::exchange(rhs.vote_, VoteImpl());
receipt_ = std::exchange(rhs.receipt_, nullptr); receipt_ = std::exchange(rhs.receipt_, nullptr);
...@@ -821,17 +818,17 @@ VoteReceipt<VoteImpl> VotingChannel<VoteImpl>::SubmitVote( ...@@ -821,17 +818,17 @@ VoteReceipt<VoteImpl> VotingChannel<VoteImpl>::SubmitVote(
template <class VoteImpl> template <class VoteImpl>
bool VotingChannel<VoteImpl>::IsValid() const { bool VotingChannel<VoteImpl>::IsValid() const {
return factory_ && voter_id_ != kInvalidVoterId<VoteImpl>; return factory_ && voter_id_;
} }
template <class VoteImpl> template <class VoteImpl>
void VotingChannel<VoteImpl>::Reset() { void VotingChannel<VoteImpl>::Reset() {
if (!factory_) if (!factory_)
return; return;
DCHECK_NE(kInvalidVoterId<VoteImpl>, voter_id_); DCHECK(voter_id_);
factory_->OnVotingChannelDestroyed(PassKey()); factory_->OnVotingChannelDestroyed(PassKey());
factory_ = nullptr; factory_ = nullptr;
voter_id_ = kInvalidVoterId<VoteImpl>; voter_id_ = VoterId<VoteImpl>();
} }
template <class VoteImpl> template <class VoteImpl>
...@@ -845,7 +842,7 @@ template <class VoteImpl> ...@@ -845,7 +842,7 @@ template <class VoteImpl>
void VotingChannel<VoteImpl>::Take(VotingChannel<VoteImpl>&& rhs) { void VotingChannel<VoteImpl>::Take(VotingChannel<VoteImpl>&& rhs) {
Reset(); Reset();
factory_ = std::exchange(rhs.factory_, nullptr); factory_ = std::exchange(rhs.factory_, nullptr);
voter_id_ = std::exchange(rhs.voter_id_, kInvalidVoterId<VoteImpl>); voter_id_ = std::exchange(rhs.voter_id_, VoterId<VoteImpl>());
} }
///////////////////////////////////////////////////////////////////// /////////////////////////////////////////////////////////////////////
...@@ -869,8 +866,12 @@ VotingChannel<VoteImpl> VotingChannelFactory<VoteImpl>::BuildVotingChannel() { ...@@ -869,8 +866,12 @@ VotingChannel<VoteImpl> VotingChannelFactory<VoteImpl>::BuildVotingChannel() {
++voting_channels_outstanding_; ++voting_channels_outstanding_;
// TODO(sebmarchand): Use VoterId<VoteImpl>::Generator instead of // TODO(sebmarchand): Use VoterId<VoteImpl>::Generator instead of
// FromUnsafeValue. // FromUnsafeValue.
// Note: The pre-increment operator is used so that the value of the first
// voter ID is 1. This is required because 0 is the value for an invalid
// VoterId.
VoterId<VoteImpl> new_voter_id = VoterId<VoteImpl> new_voter_id =
VoterId<VoteImpl>::FromUnsafeValue(++voting_channels_issued_); VoterId<VoteImpl>::FromUnsafeValue(++voting_channels_issued_);
DCHECK(!new_voter_id.is_null());
return VotingChannel<VoteImpl>( return VotingChannel<VoteImpl>(
base::PassKey<VotingChannelFactory<VoteImpl>>(), this, new_voter_id); base::PassKey<VotingChannelFactory<VoteImpl>>(), this, new_voter_id);
} }
......
...@@ -71,8 +71,7 @@ TEST(VotingTest, VoteReceiptsWork) { ...@@ -71,8 +71,7 @@ TEST(VotingTest, VoteReceiptsWork) {
voter.SetVotingChannel(consumer.voting_channel_factory_.BuildVotingChannel()); voter.SetVotingChannel(consumer.voting_channel_factory_.BuildVotingChannel());
EXPECT_EQ(&consumer.voting_channel_factory_, EXPECT_EQ(&consumer.voting_channel_factory_,
voter.voting_channel_.factory_for_testing()); voter.voting_channel_.factory_for_testing());
EXPECT_NE(voting::kInvalidVoterId<TestVote>, EXPECT_TRUE(voter.voting_channel_.voter_id());
voter.voting_channel_.voter_id());
EXPECT_TRUE(voter.voting_channel_.IsValid()); EXPECT_TRUE(voter.voting_channel_.IsValid());
voter.EmitVote(kDummyContext1, 0); voter.EmitVote(kDummyContext1, 0);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment