Skip to content

Commit

Permalink
Use bool HasHasbits(const FieldDescriptor*) instead of manual checks.
Browse files Browse the repository at this point in the history
It is not sufficient to check schema_.HasHasbits() followed by naively skipping
repeated and oneof fields as that can miss proto3 messages with message fields
and other scalar fields without "optional" keyword.

Use internal::cpp::HasHasbits(const FieldDescriptor*) instead.

PiperOrigin-RevId: 633633184
  • Loading branch information
protobuf-github-bot authored and copybara-github committed May 14, 2024
1 parent 30a40ee commit 448e326
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 3 deletions.
5 changes: 2 additions & 3 deletions src/google/protobuf/generated_message_reflection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1265,10 +1265,9 @@ void Reflection::InternalSwap(Message* lhs, Message* rhs) const {
int fields_with_has_bits = 0;
for (int i = 0; i < descriptor_->field_count(); i++) {
const FieldDescriptor* field = descriptor_->field(i);
if (field->is_repeated() || schema_.InRealOneof(field)) {
continue;
if (internal::cpp::HasHasbit(field)) {
++fields_with_has_bits;
}
fields_with_has_bits++;
}
int has_bits_size = (fields_with_has_bits + 31) / 32;
Expand Down
8 changes: 8 additions & 0 deletions src/google/protobuf/generated_message_reflection_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "google/protobuf/unittest.pb.h"
#include "google/protobuf/unittest_mset.pb.h"
#include "google/protobuf/unittest_mset_wire_format.pb.h"
#include "google/protobuf/unittest_proto3.pb.h"

// Must be included last.
#include "google/protobuf/port_def.inc"
Expand Down Expand Up @@ -1756,6 +1757,13 @@ TEST(GeneratedMessageReflection, ListFieldsSorted) {
Pointee(Property(&FieldDescriptor::number, 101))));
}

TEST(GeneratedMessageReflection, SwapImplicitPresenceShouldWork) {
proto3_unittest::TestHasbits lhs, rhs;
rhs.mutable_child()->set_optional_int32(-1);
lhs.GetReflection()->Swap(&lhs, &rhs);
EXPECT_EQ(lhs.child().optional_int32(), -1);
}

} // namespace
} // namespace protobuf
} // namespace google
76 changes: 76 additions & 0 deletions src/google/protobuf/unittest_proto3.proto
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,79 @@ message TestOneof2 {
BAZ = 3;
}
}

// If bool fields are incorrectly assumed to have hasbits, InternalSwap would
// result in swapping N more 32bit hasbits incorrectly. Considering padding, we
// need many bool fields to stress this.
message TestHasbits {
bool b1 = 1;
bool b2 = 2;
bool b3 = 3;
bool b4 = 4;
bool b5 = 5;
bool b6 = 6;
bool b7 = 7;
bool b8 = 8;
bool b9 = 9;
bool b10 = 10;
bool b11 = 11;
bool b12 = 12;
bool b13 = 13;
bool b14 = 14;
bool b15 = 15;
bool b16 = 16;
bool b17 = 17;
bool b18 = 18;
bool b19 = 19;
bool b20 = 20;
bool b21 = 21;
bool b22 = 22;
bool b23 = 23;
bool b24 = 24;
bool b25 = 25;
bool b26 = 26;
bool b27 = 27;
bool b28 = 28;
bool b29 = 29;
bool b30 = 30;
bool b31 = 31;
bool b32 = 32;
bool b33 = 33;
bool b34 = 34;
bool b35 = 35;
bool b36 = 36;
bool b37 = 37;
bool b38 = 38;
bool b39 = 39;
bool b40 = 40;
bool b41 = 41;
bool b42 = 42;
bool b43 = 43;
bool b44 = 44;
bool b45 = 45;
bool b46 = 46;
bool b47 = 47;
bool b48 = 48;
bool b49 = 49;
bool b50 = 50;
bool b51 = 51;
bool b52 = 52;
bool b53 = 53;
bool b54 = 54;
bool b55 = 55;
bool b56 = 56;
bool b57 = 57;
bool b58 = 58;
bool b59 = 59;
bool b60 = 60;
bool b61 = 61;
bool b62 = 62;
bool b63 = 63;
bool b64 = 64;
bool b65 = 65;
bool b66 = 66;
bool b67 = 67;
bool b68 = 68;
bool b69 = 69;
TestAllTypes child = 100;
}

0 comments on commit 448e326

Please sign in to comment.