Reduced nesting in GenerateByteSize: slight readability improvements in generated code.

tonyliaoss · copybara-github · commit 162a74067058 · 2024-07-23T08:52:21.000-07:00
GenerateByteSize itself remains deeply nested, but by factoring out one part of
the loop, at least we make the part that generates UpdateByteSize a bit more
readable.

Making the callsite of MayEmitIfNonDefaultCheck less nested actually resulted
in slight readability improvements also in the generated code, namely of the
form:

@@ -10563,8 +10559,7 @@ PROTOBUF_NOINLINE void OneStringEdition:
            {
             // string data = 1;
-            cached_has_bits =
-                this_._impl_._has_bits_[0];
+            cached_has_bits = this_._impl_._has_bits_[0];
             if (cached_has_bits &amp; 0x00000001u) {
	       total_size += 1 + ::proto2::internal::WireFormatLite::StringSize(
                                               this_._internal_data());

These readability improvements should be kept IMO -- they make the generated
protobuf C++ code slightly easier to read.

PiperOrigin-RevId: 655180880
diff --git a/editions/golden/compare_cpp_codegen_failure.txt b/editions/golden/compare_cpp_codegen_failure.txt
@@ -30,9 +30,9 @@
             {
 -            // optional int32 int32_field = 1;
 +            // int32 int32_field = 1;
-             cached_has_bits =
-                 this_._impl_._has_bits_[0];
+             cached_has_bits = this_._impl_._has_bits_[0];
              if (cached_has_bits & 0x00000001u) {
+               total_size += ::_pbi::WireFormatLite::Int32SizePlusOne(
 [  FAILED  ] third_party/protobuf/editions/golden/simple_proto3.pb.cc
 [ RUN      ] third_party/protobuf/editions/golden/simple_proto3.pb.h
 @@ @@
diff --git a/editions/golden/compare_cpp_codegen_failure.xml b/editions/golden/compare_cpp_codegen_failure.xml
@@ -2,7 +2,7 @@
 <testsuites tests="1" name="AllTests">
 <testsuite name="EditionsCodegenTests">
 <testcase name="third_party/protobuf/editions/golden/simple_proto3.pb.cc" status="run" result="completed" classname="DiffTest">
-<failure message="Value of: third_party/protobuf/editions/golden/simple_proto3.pb.cc&#x0A;Expected: &#x0A;// Generated by the protocol buffer compiler.  DO NOT EDIT!&#x0A;// NO CHECKED-IN PROTOBUF GENCODE&#x0A;// source: third_party/protobuf/editions/golden/simple_proto3.proto&#x0A;// Protobuf C++ Version: 0.20240718.0&#x0A;&#x0A;#include &quot;third_party/protobuf/editions/golden/simple_proto3.pb.h&quot;&#x0A;&#x0A;#include &lt;algorithm&gt;&#x0A;#include &lt;type_traits&gt;&#x0A;#include &quot;third_party/protobuf/io/coded_stream.h&quot;&#x0A;#include &quot;third_party/protobuf/generated_message_tctable_impl.h&quot;&#x0A;#include &quot;third_party/protobuf/extension_set.h&quot;&#x0A;#include &quot;third_party/protobuf/wire_format_lite.h&quot;&#x0A;#include &quot;third_party/protobuf/io/zero_copy_stream_impl_lite.h&quot;&#x0A;// @@protoc_insertion_point(includes)&#x0A;&#x0A;// Must be included last.&#x0A;, with the difference:&#x0A;@@ @@&#x0A;     ::_pbi::TcParser::GetTable&lt;::protobuf_editions_test::golden::SimpleProto3&gt;(),  // to_prefetch&#x0A;     #endif  // PROTOBUF_PREFETCH_PARSE_TABLE&#x0A;   }, {{&#x0A;-    // optional int32 int32_field = 1;&#x0A;+    // int32 int32_field = 1;&#x0A;     {::_pbi::TcParser::FastV32S1,&#x0A;      {8, 0, 0, PROTOBUF_FIELD_OFFSET(SimpleProto3, _impl_.int32_field_)}},&#x0A;   }}, {{&#x0A;     65535, 65535&#x0A;   }}, {{&#x0A;-    // optional int32 int32_field = 1;&#x0A;+    // int32 int32_field = 1;&#x0A;     {PROTOBUF_FIELD_OFFSET(SimpleProto3, _impl_.int32_field_), _Internal::kHasBitsOffset + 0, 0,&#x0A;     (0 | ::_fl::kFcOptional | ::_fl::kInt32)},&#x0A;   }},&#x0A;@@ @@&#x0A;           (void)cached_has_bits;&#x0A; &#x0A;           cached_has_bits = this_._impl_._has_bits_[0];&#x0A;-          // optional int32 int32_field = 1;&#x0A;+          // int32 int32_field = 1;&#x0A;           if (cached_has_bits &amp; 0x00000001u) {&#x0A;             target = ::proto2::internal::WireFormatLite::&#x0A;                 WriteInt32ToArrayWithField&lt;1&gt;(&#x0A;@@ @@&#x0A;           (void)cached_has_bits;&#x0A; &#x0A;            {&#x0A;-            // optional int32 int32_field = 1;&#x0A;+            // int32 int32_field = 1;&#x0A;             cached_has_bits =&#x0A;                 this_._impl_._has_bits_[0];&#x0A;             if (cached_has_bits &amp; 0x00000001u) {" type=""></failure>
+<failure message="Value of: third_party/protobuf/editions/golden/simple_proto3.pb.cc&#x0A;Expected: &#x0A;// Generated by the protocol buffer compiler.  DO NOT EDIT!&#x0A;// NO CHECKED-IN PROTOBUF GENCODE&#x0A;// source: third_party/protobuf/editions/golden/simple_proto3.proto&#x0A;// Protobuf C++ Version: 0.20240718.0&#x0A;&#x0A;#include &quot;third_party/protobuf/editions/golden/simple_proto3.pb.h&quot;&#x0A;&#x0A;#include &lt;algorithm&gt;&#x0A;#include &lt;type_traits&gt;&#x0A;#include &quot;third_party/protobuf/io/coded_stream.h&quot;&#x0A;#include &quot;third_party/protobuf/generated_message_tctable_impl.h&quot;&#x0A;#include &quot;third_party/protobuf/extension_set.h&quot;&#x0A;#include &quot;third_party/protobuf/wire_format_lite.h&quot;&#x0A;#include &quot;third_party/protobuf/io/zero_copy_stream_impl_lite.h&quot;&#x0A;// @@protoc_insertion_point(includes)&#x0A;&#x0A;// Must be included last.&#x0A;, with the difference:&#x0A;@@ @@&#x0A;     ::_pbi::TcParser::GetTable&lt;::protobuf_editions_test::golden::SimpleProto3&gt;(),  // to_prefetch&#x0A;     #endif  // PROTOBUF_PREFETCH_PARSE_TABLE&#x0A;   }, {{&#x0A;-    // optional int32 int32_field = 1;&#x0A;+    // int32 int32_field = 1;&#x0A;     {::_pbi::TcParser::FastV32S1,&#x0A;      {8, 0, 0, PROTOBUF_FIELD_OFFSET(SimpleProto3, _impl_.int32_field_)}},&#x0A;   }}, {{&#x0A;     65535, 65535&#x0A;   }}, {{&#x0A;-    // optional int32 int32_field = 1;&#x0A;+    // int32 int32_field = 1;&#x0A;     {PROTOBUF_FIELD_OFFSET(SimpleProto3, _impl_.int32_field_), _Internal::kHasBitsOffset + 0, 0,&#x0A;     (0 | ::_fl::kFcOptional | ::_fl::kInt32)},&#x0A;   }},&#x0A;@@ @@&#x0A;           (void)cached_has_bits;&#x0A; &#x0A;           cached_has_bits = this_._impl_._has_bits_[0];&#x0A;-          // optional int32 int32_field = 1;&#x0A;+          // int32 int32_field = 1;&#x0A;           if (cached_has_bits &amp; 0x00000001u) {&#x0A;             target = ::proto2::internal::WireFormatLite::&#x0A;                 WriteInt32ToArrayWithField&lt;1&gt;(&#x0A;@@ @@&#x0A;           (void)cached_has_bits;&#x0A; &#x0A;            {&#x0A;-            // optional int32 int32_field = 1;&#x0A;+            // int32 int32_field = 1;&#x0A;             cached_has_bits = this_._impl_._has_bits_[0];&#x0A;             if (cached_has_bits &amp; 0x00000001u) {&#x0A;               total_size += ::_pbi::WireFormatLite::Int32SizePlusOne(" type=""></failure>
 </testcase>
 <testcase name="third_party/protobuf/editions/golden/simple_proto3.pb.h" status="run" result="completed" classname="DiffTest">
 <failure message="Value of: third_party/protobuf/editions/golden/simple_proto3.pb.h&#x0A;Expected: &#x0A;// Generated by the protocol buffer compiler.  DO NOT EDIT!&#x0A;// NO CHECKED-IN PROTOBUF GENCODE&#x0A;// source: third_party/protobuf/editions/golden/simple_proto3.proto&#x0A;// Protobuf C++ Version: 0.20240718.0&#x0A;&#x0A;#ifndef GOOGLE_PROTOBUF_INCLUDED_third_5fparty_2fprotobuf_2feditions_2fgolden_2fsimple_5fproto3_2eproto_2epb_2eh&#x0A;#define GOOGLE_PROTOBUF_INCLUDED_third_5fparty_2fprotobuf_2feditions_2fgolden_2fsimple_5fproto3_2eproto_2epb_2eh&#x0A;&#x0A;#include &lt;limits&gt;&#x0A;#include &lt;string&gt;&#x0A;#include &lt;type_traits&gt;&#x0A;#include &lt;utility&gt;&#x0A;&#x0A;#include &quot;third_party/protobuf/runtime_version.h&quot;&#x0A;#if PROTOBUF_VERSION != 20240718&#x0A;#error &quot;Protobuf C++ gencode is built with an incompatible version of&quot;&#x0A;#error &quot;Protobuf C++ headers/runtime. See&quot;&#x0A;#error &quot;https://protobuf.dev/support/cross-version-runtime-guarantee/#cpp&quot;&#x0A;#endif&#x0A;#include &quot;third_party/protobuf/io/coded_stream.h&quot;&#x0A;#include &quot;third_party/protobuf/arena.h&quot;&#x0A;#include &quot;third_party/protobuf/arenastring.h&quot;&#x0A;#include &quot;third_party/protobuf/generated_message_tctable_decl.h&quot;&#x0A;#include &quot;third_party/protobuf/generated_message_util.h&quot;&#x0A;#include &quot;third_party/protobuf/metadata_lite.h&quot;&#x0A;#include &quot;third_party/protobuf/message_lite.h&quot;&#x0A;// @@protoc_insertion_point(includes)&#x0A;&#x0A;// Must be included last.&#x0A;&#x0A;#endif  // GOOGLE_PROTOBUF_INCLUDED_third_5fparty_2fprotobuf_2feditions_2fgolden_2fsimple_5fproto3_2eproto_2epb_2eh&#x0A;, with the difference:&#x0A;@@ @@&#x0A;   enum : int {&#x0A;     kInt32FieldFieldNumber = 1,&#x0A;   };&#x0A;-  // optional int32 int32_field = 1;&#x0A;+  // int32 int32_field = 1;&#x0A;   bool has_int32_field() const;&#x0A;   void clear_int32_field() ;&#x0A;   ::int32_t int32_field() const;&#x0A;@@ @@&#x0A; &#x0A; // SimpleProto3&#x0A; &#x0A;-// optional int32 int32_field = 1;&#x0A;+// int32 int32_field = 1;&#x0A; inline bool SimpleProto3::has_int32_field() const {&#x0A;   bool value = (_impl_._has_bits_[0] &amp; 0x00000001u) != 0;&#x0A;   return value;" type=""></failure>
diff --git a/src/google/protobuf/compiler/cpp/message.cc b/src/google/protobuf/compiler/cpp/message.cc
@@ -1213,6 +1213,55 @@ class AccessorVerifier {
 
 }  // namespace
 
+void MessageGenerator::EmitUpdateByteSizeForField(
+    const FieldDescriptor* field, io::Printer* p,
+    int& cached_has_word_index) const {
+  p->Emit(
+      {{"comment", [&] { PrintFieldComment(Formatter{p}, field, options_); }},
+       {"update_byte_size_for_field",
+        [&] { field_generators_.get(field).GenerateByteSize(p); }},
+       {"update_cached_has_bits",
+        [&] {
+          if (!HasHasbit(field) || field->options().weak()) return;
+
+          int has_bit_index = has_bit_indices_[field->index()];
+
+          if (cached_has_word_index == (has_bit_index / 32)) return;
+
+          cached_has_word_index = (has_bit_index / 32);
+          p->Emit({{"index", cached_has_word_index}},
+                  R"cc(
+                    cached_has_bits = this_.$has_bits$[$index$];
+                  )cc");
+        }},
+       {"check_if_field_present",
+        [&] {
+          if (!HasHasbit(field)) {
+            MayEmitIfNonDefaultCheck(p, "this_.", field);
+            return;
+          }
+
+          if (field->options().weak()) {
+            p->Emit("if (has_$name$())");
+            return;
+          }
+
+          int has_bit_index = has_bit_indices_[field->index()];
+          p->Emit(
+              {{"mask", absl::StrFormat("0x%08xu",
+                                        uint32_t{1} << (has_bit_index % 32))}},
+              "if (cached_has_bits & $mask$)");
+        }}},
+      R"cc(
+        $comment$;
+        $update_cached_has_bits$;
+        $check_if_field_present$ {
+          //~ Force newline.
+          $update_byte_size_for_field$;
+        }
+      )cc");
+}
+
 void MessageGenerator::GenerateFieldAccessorDefinitions(io::Printer* p) {
   p->Emit("// $classname$\n\n");
 
@@ -4748,62 +4797,8 @@ void MessageGenerator::GenerateByteSize(io::Printer* p) {
                       // Go back and emit checks for each of the fields we
                       // processed.
                       for (const auto* field : fields) {
-                        p->Emit(
-                            {{"comment",
-                              [&] {
-                                PrintFieldComment(Formatter{p}, field,
-                                                  options_);
-                              }},
-                             {"update_byte_size_for_field",
-                              [&] {
-                                field_generators_.get(field).GenerateByteSize(
-                                    p);
-                              }},
-                             {"update_cached_has_bits",
-                              [&] {
-                                if (!HasHasbit(field) ||
-                                    field->options().weak())
-                                  return;
-                                int has_bit_index =
-                                    has_bit_indices_[field->index()];
-                                if (cached_has_word_index ==
-                                    (has_bit_index / 32))
-                                  return;
-                                cached_has_word_index = (has_bit_index / 32);
-                                p->Emit({{"index", cached_has_word_index}},
-                                        R"cc(
-                                          cached_has_bits =
-                                              this_.$has_bits$[$index$];
-                                        )cc");
-                              }},
-                             {"check_if_field_present",
-                              [&] {
-                                if (!HasHasbit(field)) {
-                                  MayEmitIfNonDefaultCheck(p, "this_.", field);
-                                  return;
-                                }
-
-                                if (field->options().weak()) {
-                                  p->Emit("if (has_$name$())");
-                                  return;
-                                }
-
-                                int has_bit_index =
-                                    has_bit_indices_[field->index()];
-                                p->Emit(
-                                    {{"mask", absl::StrFormat(
-                                                  "0x%08xu",
-                                                  1u << (has_bit_index % 32))}},
-                                    "if (cached_has_bits & $mask$)");
-                              }}},
-                            R"cc(
-                              $comment$;
-                              $update_cached_has_bits$;
-                              $check_if_field_present$ {
-                                //~ Force newline.
-                                $update_byte_size_for_field$;
-                              }
-                            )cc");
+                        EmitUpdateByteSizeForField(field, p,
+                                                   cached_has_word_index);
                       }
                     }},
                    {"may_update_cached_has_word_index",
diff --git a/src/google/protobuf/compiler/cpp/message.h b/src/google/protobuf/compiler/cpp/message.h
@@ -186,6 +186,10 @@ class MessageGenerator {
   int HasWordIndex(const FieldDescriptor* field) const;
   std::vector<uint32_t> RequiredFieldsBitMask() const;
 
+  // Helper function to reduce nesting levels of deep Emit calls.
+  void EmitUpdateByteSizeForField(const FieldDescriptor* field, io::Printer* p,
+                                  int& cached_has_word_index) const;
+
   const Descriptor* descriptor_;
   int index_in_file_messages_;
   Options options_;
diff --git a/src/google/protobuf/descriptor.pb.cc b/src/google/protobuf/descriptor.pb.cc
