deps: update protobuf to 31.1 #325
Conversation
|
I ran into an unexpected issue while reviewing this PR. Please try again later. |
WalkthroughThis update introduces several new features and improvements to the Protocol Buffers C++ codebase. Major highlights include the addition of a new field chunking and layout optimization system for C++ message generation, expanded nullability annotations throughout the Arena and Any APIs, and significant internal refactoring of message layout and parsing logic. The build system is updated with new targets, source files, and test coverage. Numerous namespace qualification and code style improvements are made, and new tests and documentation are added to support the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CodeGenerator
participant MessageLayoutHelper
participant FieldChunk
participant MessageGenerator
User->>CodeGenerator: Request code generation for proto file
CodeGenerator->>MessageGenerator: Initialize message generation
MessageGenerator->>FieldChunk: Collect and group fields by criteria
FieldChunk-->>MessageGenerator: Return field chunks
MessageGenerator->>MessageLayoutHelper: Optimize field layout using chunks
MessageLayoutHelper-->>MessageGenerator: Return optimized field order
MessageGenerator->>CodeGenerator: Generate C++ code with optimized layout
CodeGenerator-->>User: Output generated C++ code
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
deps/protobuf/src/google/protobuf/compiler/cpp/file.cc (1)
286-289:⚠️ Potential issueIncorrect
#define– variable placeholder typo will leak literal dollar sign into generated filesThe line
#define $dllexport_macro$$ dllexport_decl$expands to something like
#define PROTOBUF_EXPORT$ dllexport_declbecause the second placeholder is malformed (
$ dllexport_decl$, with a leading space) and the stray$after the first placeholder is emitted verbatim.
Down-stream headers will therefore contain the literal characters$ dllexport_declinstead of the intended expansion of thedllexport_declvariable, breaking compilation.-#define $dllexport_macro$$ dllexport_decl$ +#define $dllexport_macro$ $dllexport_decl$
🧹 Nitpick comments (25)
deps/protobuf/src/google/protobuf/bridge/message_set.proto (3)
33-33: Clarify file-level deprecation comment
The standalone// DEPRECATED: DO NOT USE FOR NEW FIELDS.before thesyntaxdeclaration may imply the entire file is deprecated, not just theMessageSetmessage. Consider specifying “MessageSet deprecated” or moving the notice closer to the message to avoid confusion.
43-49: Condense repeated deprecation block
The multi-line deprecation comment is clear, but it duplicates context and is quite verbose. You could merge into a single concise paragraph to improve readability, for example:// DEPRECATED: MessageSet is obsolete. For proto2, define your own extendable messages; // for proto3, use `Any`.
62-63: Improve extension formatting
Splitting the range and its options over two lines is valid but non-standard. For clarity, merge them into one line:- extensions 4 to max - [verification = UNVERIFIED]; + extensions 4 to max [verification = UNVERIFIED];deps/protobuf/src/google/protobuf/compiler/java/doc_comment.h (1)
15-16: Remove unnecessary C header include.The
#include <stdbool.h>is unnecessary in C++ code sinceboolis a built-in type. This header is typically used in C code.-#include <stdbool.h> -deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto.h (1)
42-44: Prefersize_tforparallelismto prevent signed/unsigned mismatches
NumCPUs()returns anint, but the value ultimately controls container sizes and loop counters that aresize_telsewhere in the codebase (RunInParallel). Storing it in anintrisks implicit casts and negative values ifNumCPUs()ever fails and returns-1.- int parallelism = NumCPUs(); + size_t parallelism = static_cast<size_t>(NumCPUs());deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto_test.cc (1)
6-7: Duplicate include – remove the secondfile.hIncluding the same header twice is redundant, increases compile time, and clutters diffs.
-#include "google/protobuf/testing/file.h"deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto.cc (1)
366-370: Logging every repeated field at INFO may flood logs
ABSL_LOG(INFO)inside the hot aggregation loop will emit one line per repeated field for every profile processed. At scale (e.g., fleet-wide aggregation) this generates large, mostly unhelpful output.Consider demoting to
VLOG(1)or gating behind an explicit verbose flag.deps/protobuf/.bazelci/README.md (1)
3-3: Fix markdown formatting for the URL.The bare URL should be formatted as a proper markdown link for better readability and consistency.
-The `.bazelci/presubmit.yml` file is used by https://buildkite.com/bazel/protobuf +The `.bazelci/presubmit.yml` file is used by [Buildkite](https://buildkite.com/bazel/protobuf)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
deps/protobuf/src/google/protobuf/compiler/command_line_interface.cc (1)
1251-1252: Validate new descriptor pool enforcement calls and test coverageWe added
EnforceOptionDependencies(true)andEnforceNamingStyle(true)by default. Confirm these align with expected CLI behavior and won’t break existing workflows. Ensure that the corresponding unit tests (e.g.,command_line_interface_unittest.cc) cover:
- rejection of invalid option dependencies
- enforcement of naming style violations
Also consider whether it’s worth exposing these as configurable flags rather than hard-coding to
true.deps/protobuf/src/google/protobuf/arenastring.cc (1)
10-18: Confirm necessity and ordering of added headersNew headers
<atomic>,<cassert>,<cstdint>,<cstring>,<string>,<utility>andabsl/base/const_init.hhave been introduced. Verify each is required (e.g.,<atomic>forstd::atomic,<cassert>forABSL_DCHECK) and remove any unused includes to keep compile times minimal.deps/protobuf/src/google/protobuf/compiler/cpp/file.cc (1)
258-262: Variable rename: make sure all call-sites were updated
inline_fnswas renamed toinline_defsin the template map, but only this location is visible here.
Search the rest of the codebase (especiallyGenerateSharedHeaderCodecallers) for the old key to avoid runtimeabsl::StrFormaterrors at code-generation time.agents/grpc/src/proto/source_code.pb.h (1)
621-678: Getter now returns a conststd::string&– consider string_view, but otherwise LGTMNothing functionally changed except the fully-qualified
::std::stringand some minor inlining tweaks.
If you ever profile this hot path you might want aabsl::string_viewaccessor instead of a reference (avoids forcing callers to copy / keeps header footprint smaller), but this is generated code so not actionable here.deps/protobuf/src/google/protobuf/compiler/cpp/extension.cc (1)
129-158: Large base64 blob in header increases compile time & object sizeEmbedding the default-data string literal directly in every translation unit that includes the header:
- Bloats object files.
- Slows down compilation (the compiler has to parse a long string literal N times).
Prefer moving the constexpr array to the generated
.ccfile and keeping only anexterndeclaration in the header.deps/protobuf/src/file_lists.cmake (1)
1462-1465: New test file for lazy dependency building.The addition of
lazily_build_dependencies_test.ccsuggests new functionality for lazy dependency building. Ensure this feature is well-documented.Would you like me to review the implementation of the lazy dependency building feature to ensure it's properly integrated?
deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.cc (1)
106-137: Add bounds checking for alignment size.While the code has a FATAL log for unknown alignment sizes, it would be better to handle this more gracefully or at least document the expected alignment sizes.
Consider adding a helper function to validate alignment size:
+constexpr bool IsValidAlignmentSize(size_t size) { + return size == 1 || size == 4 || size == 8; +} + FieldAlignmentGroups MessageLayoutHelper::BuildFieldAlignmentGroups( const FieldVector& fields, const Options& options, MessageSCCAnalyzer* scc_analyzer) const { FieldAlignmentGroups field_alignment_groups; for (const auto* field : fields) { FieldFamily f = GetFieldFamily(field, options, scc_analyzer); FieldHotness hotness = GetFieldHotnessCategory(field, options, scc_analyzer); FieldGroup fg = SingleFieldGroup(field); + const size_t alignment = EstimateAlignmentSize(field); + ABSL_CHECK(IsValidAlignmentSize(alignment)) + << "Unknown alignment size " << alignment << " for field " + << field->full_name() << "."; switch (EstimateAlignmentSize(field)) { case 1: field_alignment_groups.aligned_to_1[f][hotness].push_back(fg); break; case 4: field_alignment_groups.aligned_to_4[f][hotness].push_back(fg); break; case 8: field_alignment_groups.aligned_to_8[f][hotness].push_back(fg); break; - default: - ABSL_LOG(FATAL) << "Unknown alignment size " - << EstimateAlignmentSize(field) << "for field " - << field->full_name() << "."; } } return field_alignment_groups; }deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_editions.proto (1)
19-19: Package directory mismatch with filename.The static analysis tool correctly identifies that the package name
edition_unittestdoesn't match the directory structure. The file is located indeps/protobuf/src/google/protobuf/compiler/cpp/but should be in a directory matching the package name for consistency.Consider either:
- Moving this file to an
edition_unittestsubdirectory, or- Changing the package name to match the current directory structure (e.g.,
google.protobuf.compiler.cpp)🧰 Tools
🪛 Buf (1.54.0)
19-19: Files with package "edition_unittest" must be within a directory "edition_unittest" relative to root but were in directory "deps/protobuf/src/google/protobuf/compiler/cpp".
(PACKAGE_DIRECTORY_MATCH)
deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.h (4)
15-19: Consider documenting the added standard headers.The addition of
<array>,<cstddef>,<cstdint>, and<vector>headers suggests new functionality. Consider adding a brief comment explaining why these specific headers are now required.
54-54: Add const qualifier to EstimateMemorySize method.The comment mentions that field sizes are powers of 2, but this constraint should be documented more prominently or enforced in the code.
Consider adding a debug assertion to verify that field sizes are indeed powers of 2 when fields are added to the group.
90-99: Consider consolidating kCold and kSplit as mentioned in TODO.The TODO comment indicates that kCold and kSplit should be merged once all field types can be split. This suggests ongoing refactoring work.
Would you like me to help identify which field types still need split support and create a plan for consolidating these hotness categories?
201-209: Static helper method could benefit from unit tests.The
ConsolidateAlignedFieldGroupsmethod performs complex field grouping logic. Ensure this has comprehensive unit test coverage, especially for edge cases.Consider adding debug assertions to verify the preconditions:
static std::array<std::vector<FieldGroup>, kMaxHotness> ConsolidateAlignedFieldGroups( std::array<std::vector<FieldGroup>, kMaxHotness>& field_groups, size_t alignment, size_t target_alignment) { + ABSL_DCHECK_LT(alignment, target_alignment); + ABSL_DCHECK((alignment & (alignment - 1)) == 0); // Power of 2 + ABSL_DCHECK((target_alignment & (target_alignment - 1)) == 0); // Power of 2deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/string_view_field.cc (1)
653-656: Elegant template-based API simplification.The refactoring to use templated forwarding methods for setters and adders is a significant improvement. This reduces code duplication while maintaining type safety through perfect forwarding.
Consider adding a comment explaining the BytesTag parameter usage:
template <typename Arg_> inline void $Msg$::set_$name$(int index, Arg_&& value) { $WeakDescriptorSelfPin$; + // BytesTag is used to select the appropriate overload for byte fields $pbi$::AssignToString(*$mutable$, ::std::forward<Arg_>(value) $bytes_tag$);Also applies to: 690-701
deps/protobuf/src/google/protobuf/BUILD.bazel (1)
1573-1595: New testlazily_build_dependencies_test– mark asmanualto keep CI times stableThis test exercises lazy-build scenarios but is added without a size or tag override. For very large workspaces (like nsolid) this can add several seconds to every CI run.
-cc_test( - name = "lazily_build_dependencies_test", +cc_test( + name = "lazily_build_dependencies_test", + tags = ["manual"],Adding
manualkeeps the test available without making it part of the defaultbazel test //...sweep.deps/protobuf/src/google/protobuf/compiler/command_line_interface_unittest.cc (1)
37-38: Remove duplicate include to avoid -Werror=duplicate-include warnings
google/protobuf/testing/file.his included twice consecutively. Some build configurations compiled with-Werror(e.g. Clang) treat duplicate includes as an error. Keeping a single include is sufficient.-#include "google/protobuf/testing/file.h" +#include "google/protobuf/testing/file.h" // (kept once – duplicate removed) -#include "google/protobuf/testing/file.h"deps/protobuf/src/google/protobuf/compiler/cpp/message.cc (2)
578-580: Duplicate type-aliasusing Subadds noise
using Sub = ::google::protobuf::io::Printer::Sub;appears twice (line 71 and again at line 578).
The second declaration is redundant and may confuse code-readers or trigger a shadow-warning with some compilers.-using Sub = ::google::protobuf::io::Printer::Sub;
885-899: Header for<type_traits>should be included explicitly
std::enable_if_tis now used directly (lines 885-899, 971-989), but
<type_traits>is not included in this compilation unit.
Relying on transitive includes is brittle and may break when upstream headers
change.+#include <type_traits>Place it with the other standard headers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/logs/v1/logs_service.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/logs/v1/logs_service.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/metrics/v1/metrics_service.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/metrics/v1/metrics_service.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/trace/v1/trace_service.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/trace/v1/trace_service.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/common/v1/common.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/common/v1/common.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/logs/v1/logs.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/logs/v1/logs.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/metrics/v1/metrics.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/metrics/v1/metrics.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/resource/v1/resource.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/resource/v1/resource.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/trace/v1/trace.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/trace/v1/trace.pb.his excluded by!**/gen/**
📒 Files selected for processing (116)
agents/grpc/src/proto/asset.pb.cc(7 hunks)agents/grpc/src/proto/asset.pb.h(7 hunks)agents/grpc/src/proto/blocked_loop.pb.cc(23 hunks)agents/grpc/src/proto/blocked_loop.pb.h(14 hunks)agents/grpc/src/proto/command.pb.cc(16 hunks)agents/grpc/src/proto/command.pb.h(19 hunks)agents/grpc/src/proto/common.pb.cc(15 hunks)agents/grpc/src/proto/common.pb.h(16 hunks)agents/grpc/src/proto/exit.pb.cc(15 hunks)agents/grpc/src/proto/exit.pb.h(16 hunks)agents/grpc/src/proto/info.pb.cc(20 hunks)agents/grpc/src/proto/info.pb.h(36 hunks)agents/grpc/src/proto/metrics.pb.cc(8 hunks)agents/grpc/src/proto/metrics.pb.h(4 hunks)agents/grpc/src/proto/nsolid_service.pb.cc(6 hunks)agents/grpc/src/proto/nsolid_service.pb.h(7 hunks)agents/grpc/src/proto/packages.pb.cc(15 hunks)agents/grpc/src/proto/packages.pb.h(20 hunks)agents/grpc/src/proto/profile.pb.cc(16 hunks)agents/grpc/src/proto/profile.pb.h(7 hunks)agents/grpc/src/proto/reconfigure.pb.cc(11 hunks)agents/grpc/src/proto/reconfigure.pb.h(16 hunks)agents/grpc/src/proto/source_code.pb.cc(12 hunks)agents/grpc/src/proto/source_code.pb.h(15 hunks)agents/grpc/src/proto/startup_times.pb.cc(7 hunks)agents/grpc/src/proto/startup_times.pb.h(4 hunks)deps/protobuf/.bazelci/README.md(1 hunks)deps/protobuf/.bazelci/presubmit.yml(1 hunks)deps/protobuf/.bazelrc(1 hunks)deps/protobuf/.bcr/metadata.template.json(0 hunks)deps/protobuf/.bcr/presubmit.yml(2 hunks)deps/protobuf/protobuf.gyp(2 hunks)deps/protobuf/src/file_lists.cmake(26 hunks)deps/protobuf/src/google/protobuf/BUILD.bazel(24 hunks)deps/protobuf/src/google/protobuf/any.cc(1 hunks)deps/protobuf/src/google/protobuf/any.h(3 hunks)deps/protobuf/src/google/protobuf/any.pb.cc(8 hunks)deps/protobuf/src/google/protobuf/any.pb.h(10 hunks)deps/protobuf/src/google/protobuf/any_lite.cc(4 hunks)deps/protobuf/src/google/protobuf/api.pb.cc(21 hunks)deps/protobuf/src/google/protobuf/api.pb.h(29 hunks)deps/protobuf/src/google/protobuf/arena.cc(2 hunks)deps/protobuf/src/google/protobuf/arena.h(23 hunks)deps/protobuf/src/google/protobuf/arena_align.cc(0 hunks)deps/protobuf/src/google/protobuf/arena_cleanup.h(3 hunks)deps/protobuf/src/google/protobuf/arenastring.cc(1 hunks)deps/protobuf/src/google/protobuf/arenaz_sampler.h(3 hunks)deps/protobuf/src/google/protobuf/bridge/message_set.proto(1 hunks)deps/protobuf/src/google/protobuf/compiler/BUILD.bazel(2 hunks)deps/protobuf/src/google/protobuf/compiler/code_generator.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/code_generator.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/code_generator_unittest.cc(6 hunks)deps/protobuf/src/google/protobuf/compiler/command_line_interface.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/command_line_interface_unittest.cc(19 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/BUILD.bazel(7 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/bootstrap_unittest.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/copy_unittest.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/enum.cc(8 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/extension.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/extension.h(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field.h(4 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.h(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk_test.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/enum_field.cc(5 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/map_field.cc(4 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/message_field.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/primitive_field.cc(4 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/string_field.cc(15 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/string_view_field.cc(13 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/file.cc(8 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/helpers.cc(3 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/helpers.h(4 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/message.cc(27 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/message.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/move_unittest.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/options.h(0 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/padding_optimizer.cc(0 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/padding_optimizer.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.cc(5 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_editions.proto(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_proto2.proto(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto.cc(11 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto_main.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto_test.cc(4 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/tracker.cc(3 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/unittest.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/cpp/unittest.inc(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_doc_comment.cc(1 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_doc_comment.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_enum.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_enum_field.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_field_base.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_generator.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_helpers.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_map_field.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_message.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_message_field.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_options.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_primitive_field.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_reflection_class.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_reflection_class.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_repeated_message_field.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_source_generator_base.h(2 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h(3 hunks)deps/protobuf/src/google/protobuf/compiler/csharp/names.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/java/BUILD.bazel(4 hunks)deps/protobuf/src/google/protobuf/compiler/java/doc_comment.cc(2 hunks)deps/protobuf/src/google/protobuf/compiler/java/doc_comment.h(2 hunks)
💤 Files with no reviewable changes (4)
- deps/protobuf/.bcr/metadata.template.json
- deps/protobuf/src/google/protobuf/arena_align.cc
- deps/protobuf/src/google/protobuf/compiler/cpp/options.h
- deps/protobuf/src/google/protobuf/compiler/cpp/padding_optimizer.cc
🧰 Additional context used
🧬 Code Graph Analysis (3)
agents/grpc/src/proto/asset.pb.cc (1)
agents/grpc/src/proto/asset.pb.h (20)
Asset(78-78)Asset(78-78)Asset(79-79)Asset(89-89)Asset(91-91)Asset(91-91)Asset(92-93)Asset(92-92)Asset(197-197)Asset(198-198)Asset(199-203)Asset(409-427)Asset(409-409)Asset(428-436)Asset(428-428)Asset(437-444)_impl_(185-185)GetClassData(204-204)swap(131-131)swap(131-131)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (31)
ReconfigureBody(78-721)ReconfigureBody(81-81)ReconfigureBody(81-81)ReconfigureBody(82-82)ReconfigureBody(92-92)ReconfigureBody(94-94)ReconfigureBody(94-94)ReconfigureBody(95-96)ReconfigureBody(95-95)ReconfigureBody(200-200)ReconfigureBody(201-201)ReconfigureBody(202-206)_impl_(188-188)_impl_(549-549)ReconfigureEvent(442-442)ReconfigureEvent(442-442)ReconfigureEvent(443-443)ReconfigureEvent(453-453)ReconfigureEvent(455-455)ReconfigureEvent(455-455)ReconfigureEvent(456-457)ReconfigureEvent(456-456)ReconfigureEvent(561-561)ReconfigureEvent(562-562)ReconfigureEvent(563-567)GetClassData(207-207)GetClassData(568-568)swap(134-134)swap(134-134)swap(495-495)swap(495-495)
agents/grpc/src/proto/info.pb.cc (1)
agents/grpc/src/proto/info.pb.h (30)
InfoBody(125-823)InfoBody(128-128)InfoBody(128-128)InfoBody(129-129)InfoBody(139-139)InfoBody(141-141)InfoBody(141-141)InfoBody(142-143)InfoBody(142-142)InfoBody(247-247)InfoBody(248-248)InfoBody(249-253)InfoEvent(564-564)InfoEvent(564-564)InfoEvent(565-565)InfoEvent(575-575)InfoEvent(577-577)InfoEvent(577-577)InfoEvent(578-579)InfoEvent(578-578)InfoEvent(683-683)InfoEvent(684-684)InfoEvent(685-689)GetClassData(116-116)GetClassData(254-254)GetClassData(690-690)swap(181-181)swap(181-181)swap(617-617)swap(617-617)
🪛 markdownlint-cli2 (0.17.2)
deps/protobuf/.bazelci/README.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
🪛 Buf (1.54.0)
deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_editions.proto
19-19: Files with package "edition_unittest" must be within a directory "edition_unittest" relative to root but were in directory "deps/protobuf/src/google/protobuf/compiler/cpp".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-tarball
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: build-docs
- GitHub Check: coverage-linux
- GitHub Check: test-macOS
🔇 Additional comments (375)
deps/protobuf/src/google/protobuf/compiler/cpp/bootstrap_unittest.cc (1)
100-100: Modernize allocation to use std::make_unique
Usingstd::make_unique<std::string>()instead ofreset(new std::string)improves exception safety and readability without changing behavior.deps/protobuf/src/google/protobuf/arenaz_sampler.h (3)
8-9: Approve simplified include guard macros
The updated guard (GOOGLE_PROTOBUF_ARENAZ_SAMPLER_H__) is concise and aligns with the filename.
138-139: Approve markingswapasnoexcept
Addingnoexceptto the friendswapimproves exception safety guarantees for move operations.
238-238: Approve updated closing#endif
The closing#endifnow correctly matches the new include guard.deps/protobuf/src/google/protobuf/bridge/message_set.proto (1)
59-59: Mark MessageSet as deprecated
Applyingoption deprecated = true;at the message level is correct and ensures compilers will emit warnings on use.deps/protobuf/src/google/protobuf/compiler/java/doc_comment.h (2)
58-59: Well-designed parameter addition with backward compatibility.The addition of the
is_privateparameter with a default value offalsemaintains backward compatibility while enabling the new functionality to skip deprecated javadoc for private accessors.
65-66: Consistent parameter addition across related functions.Good consistency in adding the same
is_privateparameter to bothWriteFieldAccessorDocCommentandWriteFieldStringBytesAccessorDocCommentfunctions.deps/protobuf/src/google/protobuf/compiler/java/doc_comment.cc (4)
279-279: Function signature updated correctly.The function signature properly implements the new
is_privateparameter, matching the header declaration.
283-285: Appropriate conditional logic for skipping deprecated javadoc.The condition
if (!kdoc && !is_private)correctly implements the intended behavior - deprecated javadoc is skipped for both Kotlin docs and private accessors. This makes sense since private accessors typically don't need deprecated annotations in their documentation.
407-410: Consistent function signature update.The function signature is properly updated to match the header declaration, maintaining consistency with the related
WriteFieldAccessorDocCommentfunction.
414-416: Consistent implementation of conditional logic.The same conditional check is properly applied here, ensuring consistent behavior across both
WriteFieldAccessorDocCommentandWriteFieldStringBytesAccessorDocCommentfunctions.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_options.h (1)
8-9: Include guard updated for consistencyRenamed the macro to include an extra
CSHARP_segment, aligning with the pattern used in other C# compiler headers and avoiding potential conflicts.Also applies to: 60-60
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_doc_comment.h (1)
8-9: Include guard macro renamedUpdated the guard to reflect the full file path with a double
CSHARP_prefix, matching the naming convention across C# codegen headers.Also applies to: 33-33
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_helpers.h (1)
12-13: Include guard updated to match naming patternThe renamed macro now includes the extra
CSHARP_segment, ensuring consistency with other C# generator header guards.Also applies to: 143-143
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_doc_comment.cc (1)
41-41: Use character literal for line splittingSwitching from a string literal to a
chardelimiter ('\n') leverages the dedicated overload ofabsl::StrSplit, avoiding unnecessary string construction and improving efficiency.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_source_generator_base.h (1)
8-9: Include guard macro standardizedRenamed the guard to include an additional
CSHARP_token for clear file path reflection and consistency across all C# compiler headers.Also applies to: 47-47
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_enum.h (2)
8-9: Include guard updated for consistency
The guard macro now reflects the full path (csharp/csharp_enum.h) and matches at both#ifndefand#endif.Also applies to: 42-42
26-26: Destructor correctly markedoverride
Adding theoverridespecifier ensures the destructor signature matches the base class.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_field_base.h (2)
8-9: Include guard renamed to match file path
The new guardGOOGLE_PROTOBUF_COMPILER_CSHARP_CSHARP_FIELD_BASE_H__aligns with the header’s location and is consistent.Also applies to: 93-93
33-33: Destructor markedoverride
Explicitly overriding the base destructor improves clarity and enforces correct overriding.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_reflection_class.cc (2)
197-198: Preallocate extensions vector inWriteDescriptor
Callingextensions.reserve(file_->extension_count())avoids multiple reallocations when populating file-level extensions.
300-301: Preallocate extensions vector inWriteGeneratedCodeInfo
Usingextensions.reserve(descriptor->extension_count())optimizes vector growth for message-level extensions.deps/protobuf/src/google/protobuf/compiler/csharp/names.cc (2)
172-174: Use.empty()instead ofsize() > 0
Replacinginput.size() > 0with!input.empty()enhances readability and intent clarity.
187-190: Check string emptiness with.empty()
Swappingresult.size() > 0andinput.size() > 0for!result.empty()and!input.empty()refines the style without altering behavior.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_generator.h (2)
10-11: Include guard updated to reflect header path
The guardGOOGLE_PROTOBUF_COMPILER_CSHARP_CSHARP_GENERATOR_H__now matches the file’s location and is consistent.Also applies to: 50-50
31-31: Destructor declared withoverride
Marking~Generator()asoverrideenforces correct overriding of the base class destructor.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_reflection_class.h (3)
8-9: Improved include guard naming for consistency.The include guard macro now better reflects the file path structure by including the extra
CSHARP_segment, improving clarity and reducing potential naming conflicts.
26-26: Good modernization with explicit override specifier.Adding the
overridespecifier to the destructor makes the inheritance relationship explicit and helps prevent potential issues if the base class interface changes.
52-52: Include guard end updated consistently.The closing include guard macro correctly matches the updated opening guard.
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_message.h (3)
8-9: Consistent include guard naming improvement.The renamed include guard macro follows the same improved naming convention applied across the C# compiler headers.
28-28: Explicit override specifier for destructor.Using
overridemakes the inheritance relationship clear and follows modern C++ best practices.
70-70: Include guard closure updated correctly.The closing guard macro properly matches the updated opening guard.
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_primitive_field.h (4)
8-9: Include guard naming follows established pattern.The renamed macro maintains consistency with the other C# compiler headers.
26-26: Destructors properly marked with override.Both class destructors now explicitly indicate they override base class destructors, following modern C++ guidelines.
Also applies to: 53-53
31-43: Method declarations modernized effectively.Removing the redundant
virtualkeyword while retainingoverridefollows modern C++ style guidelines. Theoverridespecifier already implies virtual behavior and provides better compiler checking.Also applies to: 59-63
71-71: Include guard closure updated consistently.The closing macro properly reflects the updated naming convention.
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_map_field.h (4)
8-9: Include guard naming completed consistently.The final header in this set maintains the improved naming convention established across the C# compiler headers.
24-24: Destructor override specifier applied correctly.The destructor now explicitly indicates it overrides the base class, completing the modernization pattern.
29-43: Virtual method declarations follow modern style.The removal of redundant
virtualkeywords while preservingoverridespecifiers improves code clarity and follows current C++ best practices.
51-51: Include guard properly closed.The closing guard macro correctly matches the updated naming convention.
deps/protobuf/src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.h (4)
8-9: Include guard macro rename: The include guard has been updated to include the extraCSHARP_segment, aligning the macro name with the file path and maintaining consistency across C# generator headers.
23-23: Destructor markedoverride: Explicitly marking the destructor withoverrideclarifies intent and ensures correct override behavior.
28-43: Removed redundantvirtualand retainedoverrideon methods: Overridden methods now consistently useoverridewithout the redundantvirtualkeyword, following modern C++ best practices.
51-51: Closing guard comment updated: The#endifcomment matches the new guard macro, preserving consistency.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.h (4)
8-9: Include guard updated for consistency: Renamed to include the extraCSHARP_prefix, matching the file’s path and other C# headers.
26-26: Destructor declared withoverride: Improves clarity on the destructor’s override of the base class.
32-47: Cleaned up method declarations: All methods drop the redundantvirtualkeyword and retainoverride, aligning with modern style guidelines.
55-55: Closing include guard adjusted: The#endifcomment now reflects the updated macro name.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_enum_field.h (6)
8-9: Include guard rename: Guard macro now includes the extraCSHARP_segment, consistent with other C# compiler headers.
24-24: Destructor markedoverride: Explicitoverridespecifier added for improved readability and correctness.
29-33: EnumFieldGenerator methods cleaned up: Removed redundantvirtualkeywords and retainedoverrideon all method declarations.
41-41: Oneof destructoroverride: TheEnumOneofFieldGeneratordestructor is now explicitly markedoverride.
46-49: EnumOneofFieldGenerator methods modernized: All overridden methods dropvirtualand keepoverride, matching style elsewhere.
57-57: Closing guard comment corrected: Matches the new include guard macro name.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_message_field.h (6)
8-9: Include guard updated: Added the extraCSHARP_prefix to the macro for path consistency.
24-24: Destructor explicitlyoverride: Improves clarity that this destructor overrides the base.
29-42: MessageFieldGenerator methods cleaned up: All method declarations removevirtualand retainoverride, following modern C++ conventions.
49-49: MessageOneofFieldGenerator destructor override: Marked withoverrideto clarify override intent.
55-60: Oneof field methods modernized: Dropped redundantvirtualand keptoverrideon all methods.
67-67: Closing include guard aligned: The#endifcomment now matches the renamed guard macro.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h (6)
8-9: Include guard rename: Macro now includes an extraCSHARP_segment, matching file structure and other headers.
26-26: Destructoroverrideadded: Clarifies override of base destructor.
31-46: WrapperFieldGenerator methods cleaned up: Removed redundantvirtual, retainedoverrideacross all method declarations.
57-57: WrapperOneofFieldGenerator destructor override: Marked withoverridefor consistency and clarity.
62-70: Oneof wrapper methods modernized: All overridden methods now useoverridewithoutvirtual.
78-78: Closing guard comment updated: Reflects the updated include guard macro name.deps/protobuf/src/google/protobuf/compiler/csharp/csharp_repeated_message_field.h (3)
8-9: Header guard update matches file path and name.The new macro
GOOGLE_PROTOBUF_COMPILER_CSHARP_CSHARP_REPEATED_MESSAGE_FIELD_H__correctly reflects both the directory (csharp) and the filename (csharp_repeated_message_field.h). The corresponding#endifcomment was also updated.Also applies to: 55-55
26-26: Destructor markedoverrideandvirtualremoved.Switching to
~RepeatedMessageFieldGenerator() override;and dropping the redundantvirtualaligns with modern C++ best practices for override specifiers.
32-47: Removed redundantvirtualkeywords and addedoverride.All overridden methods now use the
overridespecifier alone, improving clarity and ensuring mismatches are caught at compile time.deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto_main.cc (3)
42-57: Well-designed flag additions with good defaults.The new flags enhance the tool's functionality significantly:
print_unused_thresholdandprint_optimizedprovide granular output controlout_fileenables file output redirectionsort_output_by_file_nameoffers organized output formattingparallelismwithNumCPUs()default is a smart choice for performanceThe flag documentation is clear and comprehensive.
66-82: Robust argument validation and output handling.The enhanced argument validation using
ABSL_QCHECK_EQwith a descriptive usage message is excellent. The file output handling is well-implemented with proper error checking and resource management. The choice between stdout and file output is handled cleanly.
84-106: Comprehensive options configuration and improved error handling.The options struct initialization is thorough, capturing all the new flags and providing a clean interface to the analysis functions. The conditional logic for choosing between single profile analysis and aggregated analysis is clear. Error handling has been improved with descriptive logging and proper exit codes.
deps/protobuf/src/google/protobuf/compiler/BUILD.bazel (2)
18-21: LGTM - Bootstrap compiler integration.The addition of the bootstrap_cc_library load statement aligns with the protobuf build system's integration of UPB generator components.
220-248:❓ Verification inconclusive
Verify the bootstrap build system changes work correctly.
The change from
cc_librarytobootstrap_cc_libraryfor theprotoc_libtarget is a significant build system modification. This introduces a bootstrap compilation approach with new UPB generator dependencies.Please verify that the build system changes function correctly by running:
🏁 Script executed:
#!/bin/bash # Description: Verify that the protoc_lib target builds successfully with the new bootstrap approach # Expected: Successful build of protoc_lib and its dependencies # Test building the protoc_lib target cd deps/protobuf bazel build //src/google/protobuf/compiler:protoc_lib # Test building the full protoc binary to ensure end-to-end compatibility bazel build //src/google/protobuf/compiler:protoc # Verify the bootstrap dependency is available bazel query 'deps(//src/google/protobuf/compiler:protoc_lib)' | grep upb_generatorLength of output: 501
Manual verification required: Bazel unavailable in current environment
I wasn’t able to run the Bazel commands here (no
bazelbinary), so please verify the bootstrap build changes locally:
- Build the
protoc_libtarget with the newbootstrap_cc_libraryrule.- Build the full
protocbinary end-to-end.- Confirm that the UPB generator dependency is present:
bazel query 'deps(//src/google/protobuf/compiler:protoc_lib)' | grep upb_generatordeps/protobuf/src/google/protobuf/compiler/cpp/move_unittest.cc (1)
12-12: LGTM - Consistent include addition.The addition of
unittest_import.pb.haligns with similar changes in other unittest files and likely supports expanded test coverage for imported protobuf definitions.deps/protobuf/src/google/protobuf/compiler/cpp/copy_unittest.cc (1)
13-13: LGTM - Consistent with other unittest files.The include addition matches the identical change in
move_unittest.cc, ensuring consistent access to imported protobuf definitions across test files.deps/protobuf/src/google/protobuf/compiler/cpp/unittest.cc (1)
29-29: LGTM: Test coverage improvementThe addition of
unittest_import.pb.haligns with the broader pattern of ensuring comprehensive test coverage for imported protobuf definitions mentioned in the AI summary.deps/protobuf/src/google/protobuf/compiler/cpp/extension.h (1)
94-94: LGTM: Include guard comment fixCorrectly updates the include guard comment to match the actual filename (
extension.hinstead of incorrectly referencingmessage.h).deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/message_field.cc (2)
29-30: LGTM: Port macro setupProper addition of
port_def.incto ensure consistent platform-specific macro definitions.
1067-1067: LGTM: Port macro cleanupCorrectly adds
port_undef.incto clean up platform-specific macros, completing the bracketing pattern with the correspondingport_def.incinclude.deps/protobuf/src/google/protobuf/compiler/code_generator.cc (1)
108-109: LGTM: Expanded edition check bypassAdding
"com/google/protobuf/"as an additional prefix for bypassing edition validation is consistent with the existing pattern for internal protobuf files and aligns with the enhanced edition handling in this update.deps/protobuf/.bazelrc (1)
6-8: Ensure Rust linking flag is scoped appropriately
Theexperimental_use_cc_common_linkflag is now applied globally underbuild; confirm this doesn't unintentionally affect non-Rust targets or downstream platforms (e.g., macOS where the removedBAZEL_NO_APPLE_CPP_TOOLCHAINworkaround was important).deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_proto2.proto (1)
33-35: Verify C++ codegen handlesstdmessage correctly
Introducing a nested message namedstdmay conflict with the C++stdnamespace. Ensure all generated identifiers are fully qualified (e.g.,::std) and there are no namespace collisions in the C++ output.agents/grpc/src/proto/metrics.pb.h (2)
4-4: LGTM: Protobuf version update is consistent.The version update from 6.30.2 to 6.31.1 is correctly reflected in both the comment and the runtime version check macro, ensuring proper version validation.
Also applies to: 15-15
97-97: LGTM: Explicit namespace qualification improves code robustness.The change from
std::moveto::std::moveensures unambiguous resolution to the standard library function and prevents potential namespace conflicts.Also applies to: 294-294
deps/protobuf/.bcr/presubmit.yml (1)
1-1: LGTM: Expanding platform coverage with macOS ARM64 support.Adding "macos_arm64" to the platform matrix improves test coverage across different architectures. The lint markers ensure consistency with related configuration files.
Also applies to: 5-5, 22-22
deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/map_field.cc (2)
12-12: LGTM: Adding necessary Abseil logging header.The
absl/log/absl_check.hinclude is needed for ABSL_CHECK functionality used in the code.
23-25: LGTM: Consistent port definition management pattern.The addition of
port_def.incandport_undef.incincludes follows the consistent pattern across protobuf compiler files for managing platform-specific macros and compiler settings.Also applies to: 334-334
agents/grpc/src/proto/startup_times.pb.h (3)
4-4: LGTM: Consistent protobuf version update.Version update from 6.30.2 to 6.31.1 is correctly applied in both the comment and runtime check macro.
Also applies to: 15-15
34-34: LGTM: Header include updated to align with protobuf changes.The change from
map_field_inl.htomap_field.haligns with protobuf library restructuring in version 31.1.
139-139: LGTM: Explicit namespace qualification applied consistently.The change to
::std::movemaintains consistency with namespace qualification improvements across protobuf-generated code.deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc (1)
29-31: LGTM: Consistent port definition include pattern.The addition of
port_def.incandport_undef.incfollows the established pattern across protobuf compiler files for managing platform-specific macros and compiler settings.Also applies to: 479-479
deps/protobuf/src/google/protobuf/compiler/cpp/tracker.cc (2)
225-225: LGTM: Clean refactoring to use descriptive function.The replacement of the enum comparison with
IsArenaStringPtr(field)improves code readability and likely encapsulates more complex logic for arena string pointer detection.
244-244: LGTM: Consistent application of the new string field detection.The changes consistently apply the same
IsArenaStringPtr(field)logic across both string field pointer determination and default field name handling in theStringOneofGettersfunction.Also applies to: 253-253
deps/protobuf/src/google/protobuf/compiler/cpp/enum.cc (1)
63-63: LGTM: Systematic namespace qualification improvements.The addition of explicit global namespace qualification (
::) to standard library and Abseil types ensures unambiguous references and prevents potential naming conflicts. This is consistent with the broader protobuf compiler standardization effort described in the PR.Also applies to: 172-174, 224-225, 265-265, 274-274, 359-359, 529-529, 556-556
deps/protobuf/.bazelci/presubmit.yml (1)
1-21: LGTM: Comprehensive CI configuration for protobuf verification.The Bazel CI configuration provides good coverage across platforms (debian10, macos, macos_arm64, ubuntu2004, windows) and Bazel versions (7.x, 8.x). The build targets appropriately cover core protobuf libraries, language bindings, and the protoc compiler, ensuring thorough verification of the protobuf update.
deps/protobuf/src/google/protobuf/compiler/java/BUILD.bazel (3)
75-75: LGTM: Addition of Abseil status dependency.The addition of
"@abseil-cpp//absl/status"to both thehelpersandjavalibraries suggests enhanced error handling capabilities, which is a positive improvement for the Java code generator.Also applies to: 165-165
88-88: LGTM: Build configuration alignment.Adding
copts = COPTSto theinternal_helperslibrary aligns it with other libraries in the build file, ensuring consistent compilation options across all components.
222-226: LGTM: Enhanced test coverage and dependencies.The addition of protobuf testing utilities and Abseil libraries to the
generator_unittesttarget improves test capabilities and coverage, which is beneficial for ensuring the reliability of the Java code generator.deps/protobuf/src/google/protobuf/compiler/cpp/field.h (2)
32-34: Add port_def.inc include for macro setup.
Includingport_def.incimmediately after the header includes ensures platform-specific macros are defined correctly for this translation unit.
549-550: Add port_undef.inc include for macro teardown.
Bracketing the header content withport_def.incandport_undef.inccleanly restores macros to their prior state, preventing leakage into other code.agents/grpc/src/proto/blocked_loop.pb.h (1)
4-2141: Skip review for generated code.
This header is auto-generated by the Protocol Buffers compiler. The namespace qualifications, version bump, and port includes align with the updated protobuf runtime. No manual changes required.deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/primitive_field.cc (4)
28-28: Include wire_format_lite.h for lite constants.
This import is necessary to accessWireFormatLiteconstants used in size and serialization routines.
30-31: Add port_def.inc include for macro setup.
Ensures platform-specific macros are properly defined before any implementation code.
668-669: Add port_undef.inc include for macro teardown.
Cleanly restores macro definitions after the implementation to avoid cross-file contamination.
616-616: Fully qualify size_t references.
Replaced ambiguoussize_twith::size_tfor explicit global namespace qualification, matching the surrounding protobuf C++ style.Also applies to: 637-637
deps/protobuf/src/google/protobuf/compiler/cpp/unittest.inc (2)
44-44: Header include order change looks appropriate.The placement of
gtest.haftergmock.hfollows common C++ testing conventions.
58-58: Header repositioning is consistent with test infrastructure changes.Moving
unittest_no_generic_services.pb.hlater in the include list appears to be organizational cleanup.deps/protobuf/protobuf.gyp (3)
159-159:✅ Verification successful
Verify the new field_chunk.cc source file exists.
Adding
field_chunk.ccto the C++ code generator represents new field chunking capabilities in protobuf 31.1.
🏁 Script executed:
#!/bin/bash # Description: Verify the new field_chunk.cc source file exists # Expected: File should be present in the C++ compiler directory echo "Checking for field_chunk.cc:" fd "field_chunk.cc" deps/protobuf/src/google/protobuf/compiler/cpp/ echo "Checking if it's a legitimate C++ compiler source file:" if [ -f "deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.cc" ]; then echo "File exists. First few lines:" head -10 "deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.cc" else echo "File not found!" fiLength of output: 742
field_chunk.cc confirmed
The filedeps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.ccexists and contains the expected implementation. Approving the addition of field_chunk.cc for protobuf 31.1.
67-67:✅ Verification successful
Verify the new micro_string.cc source file exists.
Adding
micro_string.ccto the protobuf static library sources appears to introduce string optimization features in protobuf 31.1.
🏁 Script executed:
#!/bin/bash # Description: Verify the new micro_string.cc source file exists # Expected: File should be present in the protobuf source directory echo "Checking for micro_string.cc:" fd "micro_string.cc" deps/protobuf/src/google/protobuf/ echo "Checking if it's a legitimate protobuf source file:" if [ -f "deps/protobuf/src/google/protobuf/micro_string.cc" ]; then echo "File exists. First few lines:" head -10 "deps/protobuf/src/google/protobuf/micro_string.cc" else echo "File not found!" fiLength of output: 825
Verified micro_string.cc presence
- deps/protobuf/src/google/protobuf/micro_string.cc exists and its contents match the expected Protobuf source.
No further action needed; approving this addition.
162-162:✅ Verification successful
Verify the new message_layout_helper.cc source file exists.
Adding
message_layout_helper.ccto the C++ code generator introduces message layout optimization features, likely replacing the removedpadding_optimizer.cc.
🏁 Script executed:
#!/bin/bash # Description: Verify the new message_layout_helper.cc source file exists and padding_optimizer.cc is removed # Expected: message_layout_helper.cc should exist, padding_optimizer.cc should not echo "Checking for message_layout_helper.cc:" fd "message_layout_helper.cc" deps/protobuf/src/google/protobuf/compiler/cpp/ echo "Checking for old padding_optimizer.cc (should not exist):" fd "padding_optimizer.cc" deps/protobuf/src/google/protobuf/compiler/cpp/ echo "Verifying message_layout_helper.cc content:" if [ -f "deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.cc" ]; then echo "File exists. First few lines:" head -10 "deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.cc" else echo "File not found!" fiLength of output: 1000
✅ Verified message_layout_helper.cc inclusion and padding_optimizer.cc removal
- deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.cc exists
- padding_optimizer.cc is no longer present in that directory
agents/grpc/src/proto/exit.pb.h (7)
4-4: Update Protobuf C++ version stamp
Header comment reflects the bumped compiler version (6.31.1) per the PR objective.
15-19: Bump Protobuf runtime version check
The#if PROTOBUF_VERSIONguard is updated to6031001, matching the new codegen/runtime version.
100-100: Qualifystd::movewith global namespace
Constructor initializer now uses::std::move, consistent with other generated code.
231-236: Fully qualifystd::stringinmessageaccessors
Public getters/setters for themessagefield now explicitly use::std::string.
238-240: Fully qualify internalmessagehelpers
Internal methods (_internal_message,_internal_set_message,_internal_mutable_message) use::std::string.
245-250: Fully qualifystd::stringinstackaccessors
Public getters/setters for thestackfield now explicitly use::std::string.
253-255: Fully qualify internalstackhelpers
Private helpers (_internal_stack,_internal_set_stack,_internal_mutable_stack) use::std::string.agents/grpc/src/proto/common.pb.h (8)
4-4: Update Protobuf C++ version stamp
Header comment updated to 6.31.1, aligning with the dependency bump.
15-19: Bump Protobuf runtime version check
PROTOBUF_VERSIONmacro updated to6031001to enforce compatibility.
98-99: Qualifystd::moveinTimemove ctor
Move constructor now uses::std::move, consistent with generated patterns.
301-302: Qualifystd::moveinErrorInfomove ctor
Constructor initializer updated to::std::movefor namespace clarity.
509-510: Qualifystd::moveinCommonResponsemove ctor
Initializer list now uses::std::move, matching the other message types.
433-442: Fully qualifystd::stringinErrorInfoaccessors
All public and internal methods for themessagefield now use::std::string.
905-914: Fully qualifystd::stringinCommonResponse::requestIdaccessors
Getter, mutable, release, and allocation methods explicitly use::std::string.
970-979: Fully qualifystd::stringinCommonResponse::commandaccessors
Getter, mutable, release, and allocation methods explicitly use::std::string.agents/grpc/src/proto/profile.pb.h (2)
4-4: Version & Runtime Check Update
Updated the Protobuf C++ version comment to 6.31.1 and adjusted thePROTOBUF_VERSIONcheck to require 6031001, matching the upgraded codegen.Also applies to: 15-15
109-109: Qualified std::move in Move Constructors
Added the global namespace qualifier forstd::movein the move constructors of all message classes, aligning with protobuf codegen conventions.Also applies to: 300-300, 515-515, 717-717, 864-864
agents/grpc/src/proto/nsolid_service.pb.h (4)
4-4: Version & Runtime Check Update
Bumped Protobuf C++ version to 6.31.1 and updated thePROTOBUF_VERSIONmacro to 6031001 to enforce compatibility with the new runtime.Also applies to: 15-15
101-101: Qualified std::move in Move Constructor
Prefixed thestd::movecall with::in theEventResponsemove constructor, following the updated code generation pattern.
230-235: Qualified std::string in API & Helper Declarations
Updated allstd::stringreferences in both public API and private helper method signatures to::std::string, ensuring full namespace qualification.Also applies to: 238-240
300-300: Qualified std::string in Method Definitions
All definitions forerror_messagegetters/setters and related helpers now explicitly use::std::string, matching the header declarations.Also applies to: 312-312, 314-314, 318-318, 322-322, 327-327, 332-332, 345-345
agents/grpc/src/proto/packages.pb.h (3)
4-6: Version update to Protobuf 6.31.1 is correct.
The version string andPROTOBUF_VERSIONcheck have been updated to6.31.1and6031001respectively, matching the PR’s objective of bumping the protobuf dependency.Also applies to: 15-19
100-100: Explicit::std::movequalification.
The move constructor now uses::std::move, ensuring the standard library overload is selected unambiguously.
239-247: Consistent global namespace qualification forstd::string.
All generated accessors and mutators for string fields (dependencies,path,name,version,main) now explicitly use::std::string(and related STL types). This avoids ambiguity with custom types and aligns with the broader code-generation changes.Also applies to: 256-262, 271-279, 285-293, 301-309, 1310-1316
deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/string_field.cc (3)
28-29: Insertport_def.inc.
Includinggoogle/protobuf/port_def.incat the end of the include block ensures platform-specific macros are defined before this file’s content.
995-997: Insertport_undef.inc.
Includinggoogle/protobuf/port_undef.incat the file’s end correctly undefines macros introduced byport_def.inc, preventing macro leakage.
220-221: Global namespace qualification for standard library symbols.
References tostd::true_type,std::string, and forwarding utilities have been updated to::std::true_type,::std::string, and::std::forward. This explicit qualification eliminates ambiguity and maintains consistency with the rest of the codebase.Also applies to: 273-286, 837-842
deps/protobuf/src/google/protobuf/api.pb.h (6)
4-4: Protobuf version bump and runtime check updated
The generated header now specifies C++ gencode version 6.31.1 and checksPROTOBUF_VERSION != 6031001, matching the upgraded dependency.Also applies to: 15-15
100-100: Explicit global::std::movein move constructors
Move constructors inMixin,Method, andApinow invoke::std::move(...), ensuring they reference the standard library overload unambiguously.Also applies to: 313-313, 598-598
229-256: Qualifiedstd::stringinMixinclass declarations
AllMixinaccessor and mutator declarations now use::std::string, eliminating any potential namespace ambiguity in the generated code.
900-1106: Qualifiedstd::stringinApiinline definitions
Inline methods forApi::nameandApi::versionnow consistently use::std::stringin return types and parameters.
1297-1508: Qualifiedstd::stringinMethodinline definitions
Inline getters and setters forMethod::name,request_type_url, andresponse_type_urlare now explicitly using::std::string.
1614-1671: Qualifiedstd::stringinMixininline methods
TheMixinclass’s inline field accessors (nameandroot) now uniformly use::std::stringfor clarity and consistency.deps/protobuf/src/google/protobuf/compiler/cpp/helpers.cc (3)
1183-1183: LGTM: Modern API usage.Good modernization to use the direct
is_repeated()method instead of label comparison, which is cleaner and more readable.
1215-1233: LGTM: Well-implemented string field detection.The new
HasRegularStringFieldsfunctions properly:
- Use
IsString()to identify string fields (excluding cord and string piece types as mentioned in the summary)- Recursively traverse nested types for comprehensive detection
- Follow consistent patterns with other similar functions in the file
These functions support the conditional inclusion of string_view_migration.h mentioned in the AI summary.
1794-1810: LGTM: Correct field filtering logic.The
CollectFieldsExcludingWeakAndOneoffunction is well-implemented:
- Properly filters out weak fields using
IsWeak(field, options)- Correctly excludes oneof fields with
!field->real_containing_oneof()- Returns the count of weak fields for layout optimization purposes
- Uses appropriate iteration with
FieldRange(d)This supports the message layout optimization mentioned in the summary.
deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/enum_field.cc (3)
28-30: LGTM: Proper port macro inclusion.Correctly includes
port_def.incat the beginning with the standard "Must be included last" comment, following protobuf patterns for platform-specific macro definitions.
494-495: LGTM: Consistent namespace qualification.Good practice to explicitly use
::size_tinstead ofstd::size_tto refer to the global namespace version, avoiding potential ambiguity. This change is consistent with the broader namespace qualification improvements mentioned in the summary.Also applies to: 498-502, 552-552, 559-559, 561-561
583-584: LGTM: Proper port macro cleanup.Correctly includes
port_undef.incat the end to bracket the implementation and clean up platform-specific macros, completing the proper macro management pattern.agents/grpc/src/proto/command.pb.h (8)
4-4: Protobuf compiler version bump
Updated the generated header’s Protobuf C++ version comment to 6.31.1, matching the upgrade target.
15-19: Runtime version check updated
ThePROTOBUF_VERSIONmacro is now6031001to enforce compatibility with the 6.31.1 runtime. This will cause a clear error if headers/runtime versions mismatch.
102-102: Qualified std::move
The move constructor uses::std::movenow, preventing any local macro or namespace collisions.
232-242: Qualified std::string in CommandResponse declarations
String accessors inCommandResponsehave been updated to use::std::string, ensuring consistent global‐namespace qualification.
1042-1087: Qualified std::string in CommandRequest.requestId
AllrequestIdaccessor and mutator definitions now fully qualify::std::string, aligning with the new code‐generation style.
1107-1152: Qualified std::string in CommandRequest.id
Theidfield’s getters and setters consistently use::std::string. Good consistency across generated methods.
1172-1217: Qualified std::string in CommandRequest.command
Accessor methods for thecommandfield now fully qualify::std::string, matching other updated patterns.
1363-1408: Qualified std::string in CommandResponse.message definitions
The implementations ofmessagefield methods are updated to use::std::string. This aligns with the declaration changes above.agents/grpc/src/proto/info.pb.h (4)
4-4: LGTM: Protobuf version update is consistent.The protobuf version has been correctly updated from 6.30.2 to 6.31.1 with consistent changes to both the comment and runtime version check. The include header change from
map_field_inl.htomap_field.haligns with protobuf library evolution.Also applies to: 15-15, 34-34
143-143: LGTM: Global namespace qualification for std::move improves safety.The explicit
::std::movequalification ensures the standard library function is used regardless of any local namespace declarations that might shadowstd. This is a good defensive practice for generated code.Also applies to: 252-252, 579-579, 688-688
292-300: LGTM: Systematic namespace qualification for std::string types.The explicit
::std::stringqualification throughout method signatures, return types, and template parameters improves namespace safety. This prevents potential conflicts with user-defined string types and follows best practices for generated code that may be included in diverse namespace contexts.Also applies to: 324-335, 533-533, 797-797, 809-842
1478-1479: LGTM: Global namespace qualification for std::forward enhances perfect forwarding safety.The explicit
::std::forwardqualification in template forwarding contexts prevents potential ADL issues and ensures the standard library's perfect forwarding utility is used consistently.Also applies to: 1486-1486
deps/protobuf/src/google/protobuf/compiler/cpp/file.cc (1)
50-51: Include order caution
port.hmust be included before any proto headers that rely on its portability macros.
You inserted it just beforeport_def.inc, which is correct.
Good catch.deps/protobuf/src/google/protobuf/compiler/code_generator.h (1)
146-158: New helper API looks sound
GetResolvedSourceFeatureExtension()cleanly delegates toInternalFeatureHelperand keeps the template parameter list consistent with the existing helpers.
No issues spotted.agents/grpc/src/proto/reconfigure.pb.h (2)
14-20: Generated version guard updated correctlyThe runtime check now expects
6031001, matching protoc 6.31.1.
As long as the runtime library is upgraded in lock-step, this is fine.Please ensure the build environment ships
libprotobuf6.31.1; otherwise this guard will hard-error.
241-254: Namespace qualification change is benignChanging
std::stringto::std::stringreduces ADL surprises and matches modern protoc output.
Looks good.agents/grpc/src/proto/nsolid_service.pb.cc (1)
4-4: LGTM! Protobuf version upgrade changes are consistent.The systematic updates reflect the protobuf compiler upgrade from 6.30.2 to 6.31.1, including internal metadata optimizations, namespace qualification improvements, and weak linkage attributes. These are expected auto-generated changes with no functional impact.
Also applies to: 63-65, 72-72, 145-145, 240-241, 316-316, 391-391
deps/protobuf/src/google/protobuf/any.pb.cc (1)
4-4: LGTM! Core protobuf Any message updated consistently.The changes to the protobuf Any message align with the version 6.31.1 upgrade, including proper namespace qualification (
::std::) and internal metadata optimizations. The weak linkage attribute onGetClassData()follows the new compiler patterns.Also applies to: 72-74, 83-83, 132-132, 230-231, 317-317, 327-327, 419-419
agents/grpc/src/proto/metrics.pb.cc (1)
4-4: LGTM! Multiple message types updated consistently.Both MetricsBody and MetricsEvent messages show consistent protobuf 6.31.1 upgrade patterns, including bitmap/hasbit offset adjustments, migration schema simplifications, namespace qualifications, and weak attribute additions to
GetClassData()methods.Also applies to: 87-96, 100-101, 251-252, 389-389, 402-402, 514-515, 697-697
agents/grpc/src/proto/asset.pb.h (1)
4-4: LGTM! Header file API updates maintain compatibility.The protobuf header shows appropriate version updates and systematic namespace qualification changes (
std::→::std::) for string types and move operations. The runtime version check ensures compatibility between generated code and runtime library.Also applies to: 15-15, 93-93, 202-202, 229-241, 596-596, 608-608, 614-614, 618-618, 623-623, 628-628, 641-641
deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.cc (4)
17-20: LGTM! Clean helper function with proper bounds checking.The
GetHasWordIdxfunction correctly calculates the 32-bit word index for hasbit tracking. Thestatic_cast<size_t>ensures safe array indexing.
25-35: LGTM! Robust validation with helpful debug checks.The function correctly validates that all fields share the same hasbit word index, with debug assertions to catch inconsistencies early. The empty check prevents undefined behavior.
57-71: LGTM! Correct bitmask generation with comprehensive validation.The mask generation logic correctly:
- Handles empty input gracefully
- Validates consistent word indices across fields
- Generates proper bit positions using modulo arithmetic
- Ensures non-zero output with final assertion
75-88: LGTM! Well-designed overload for chunk iterator ranges.The overloaded function appropriately:
- Validates non-empty iterator range
- Delegates consistency checking to helper function
- Accumulates masks from multiple chunks using bitwise OR
- Handles empty chunks gracefully in the loop
deps/protobuf/src/google/protobuf/compiler/cpp/helpers.h (5)
369-370: LGTM: Proper symbol export for IsRarelyPresent function.Adding
PROTOC_EXPORTensures this function is properly exported from the protobuf compiler library, which is essential for external usage.
460-464: LGTM: Consistent export macros for ShouldSplit overloads.Both overloads of
ShouldSplitnow have properPROTOC_EXPORTmacros, ensuring consistent symbol visibility across the API.
490-492: LGTM: New helper function for regular string field detection.The
HasRegularStringFieldsfunction provides a useful utility for checking string/bytes fields while excluding cord and string piece fields. The documentation clearly explains the exclusions.
499-499: LGTM: Corrected comment reference.The comment correction from
map_field_inl.htomap_field.haligns with the actual header file that needs to be included for map field functionality.
735-741: LGTM: New utility function for field collection.The
CollectFieldsExcludingWeakAndOneoffunction provides a useful abstraction for gathering fields while excluding weak fields and oneof fields. The return of weak field count is a practical design for callers who need this information.agents/grpc/src/proto/asset.pb.cc (7)
4-4: LGTM: Version update reflects protobuf upgrade.The version update from 6.30.2 to 6.31.1 correctly reflects the protobuf dependency upgrade mentioned in the PR objectives.
70-72: LGTM: Internal metadata structure updates.The changes to the offsets array (adding bitmap
0x081and replacing~0uwith concrete hasbit index11) are typical internal optimizations from the protobuf compiler upgrade. These affect layout optimization but don't change the public API.
93-93: LGTM: Simplified migration schema entry.The migration schema simplification to only specify the size of the
Assetstruct is a reasonable optimization in the generated code.
136-136: LGTM: Namespace qualification improvement.Using fully qualified
::std::declvalinstead of unqualifiedstd::declvalprevents potential namespace conflicts and is a good practice in generated code.
263-264: LGTM: Weak attribute and signature formatting.Adding
PROTOBUF_ATTRIBUTE_WEAKtoGetClassData()allows for better link-time optimization, and splitting the signature across lines improves readability.
423-423: LGTM: Consistent namespace qualification.Using fully qualified
::std::stringprevents potential namespace conflicts in generated code, consistent with other similar changes in the file.
621-621: LGTM: Global namespace qualification for swap.Using
::std::swapinstead ofstd::swapensures the standard library swap is used regardless of any potential local namespace declarations.deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk_test.cc (5)
1-42: LGTM: Well-structured test infrastructure.The helper functions
CreateFieldArrayandCreateAndCollectFieldsprovide clean abstractions for test setup, making the tests more readable and maintainable. The use of templates allows for flexibility in testing different proto types.
43-85: LGTM: Comprehensive field collection tests.The tests cover different chunking scenarios effectively:
SingleChunk: Tests when all fields group togetherAllDifferentChunks: Tests when no fields group togetherRepeatedAndSingular: Tests realistic grouping by field characteristicsThe assertions properly validate both chunk count and field characteristics.
86-111: LGTM: Thorough chunk comparison tests.The
FindNextUnequalChunktests validate both edge cases (all equal, all unequal) with appropriate predicates and verify that the returned iterator points to the correct chunk.
113-146: LGTM: Complete field extraction tests.The extraction tests validate both negative case (extract none) and positive case (extract all), properly checking that:
- Original chunks are modified (fields removed)
- Extracted fields match the expected set
- Empty chunks remain after extraction
148-178: LGTM: Accurate bit mask generation tests.The bit mask generation tests validate:
- Single field mask generation with correct bit position
- Multiple chunk mask generation with OR-ed bits
- Proper use of field indices and hasbit indices
The test assertions verify the exact bit patterns expected.
agents/grpc/src/proto/reconfigure.pb.cc (6)
4-4: Protobuf version upgrade applied correctly.The version header has been properly updated to reflect the protobuf upgrade from 6.30.2 to 6.31.1.
104-106: Improved offset table efficiency.The replacement of placeholder entries (
~0u) with explicit bitmap and hasbit index offsets (0x081, 15, 5) improves runtime performance by reducing indirection in the internal protobuf metadata.Also applies to: 130-133
142-143: Schema simplification improves efficiency.The migration schema entries have been streamlined to include only essential data (start offset and size), reducing memory footprint and improving cache performance.
325-326: Added weak linking support for better library compatibility.The
PROTOBUF_ATTRIBUTE_WEAKattribute onGetClassData()methods enables weak linking, which helps avoid duplicate symbol issues when linking multiple protobuf-generated files and improves dynamic library compatibility.Also applies to: 851-852
528-528: Improved namespace qualification for standard library types.The change from
std::stringto::std::stringprovides explicit global namespace qualification, preventing potential conflicts with user-defined types and ensuring unambiguous reference to the standard library.Also applies to: 536-536, 544-544
714-714: Consistent global namespace qualification for swap function.The change to
using ::std::swap;maintains consistency with other namespace qualifications and ensures the standard library swap function is used, avoiding potential conflicts with user-defined swap functions.Also applies to: 1034-1034
deps/protobuf/src/google/protobuf/compiler/code_generator_unittest.cc (3)
70-70: Properly exposed protected method for testing.The
usingdeclaration appropriately exposes the protectedGetResolvedSourceFeatureExtensionmethod for unit testing, following standard testing patterns for accessing protected members.
250-385: Comprehensive test coverage for new feature extension functionality.The new test methods provide excellent coverage of
GetResolvedSourceFeatureExtension:
- Basic functionality test: Verifies consistency with
GetResolvedSourceFeatures().GetExtension()- Custom defaults test: Ensures proper behavior with modified feature set defaults
- Edge case test: Validates behavior when pool doesn't provide defaults vs. when feature extensions do
The tests are well-structured with clear assertions and cover important scenarios for the new feature resolution functionality.
422-422: Updated test data reflects new protobuf features.The addition of
default_symbol_visibilityfield with appropriate values (EXPORT_ALL,EXPORT_TOP_LEVEL) in the test expectations correctly reflects the new feature support in the protobuf version upgrade.Also applies to: 436-436, 449-452, 464-464
agents/grpc/src/proto/packages.pb.cc (4)
4-4: Consistent protobuf upgrade improvements applied.The version update to 6.31.1 and offset table improvements with explicit bitmap values (0x081, 0x000) follow the same beneficial pattern seen in other protobuf-generated files, ensuring consistency across the codebase.
Also applies to: 125-127, 132-137
153-155: Schema optimization consistently applied.The migration schema simplification mirrors the improvements in other protobuf files, reducing memory usage and improving performance across the entire generated codebase.
312-313: Weak linking support added consistently across all message classes.The
PROTOBUF_ATTRIBUTE_WEAKattribute has been properly added to allGetClassData()methods (PackagesEvent, PackagesBody, Package), ensuring consistent linking behavior across all protobuf-generated classes.Also applies to: 615-616, 886-887
495-495: Global namespace qualification applied consistently.The changes from
std::to::std::for swap functions and string types maintain consistency with other protobuf files and provide explicit global namespace qualification to prevent potential naming conflicts.Also applies to: 753-753, 1011-1011, 1021-1021, 1031-1031, 1041-1041, 1207-1207
agents/grpc/src/proto/command.pb.cc (13)
4-4: LGTM! Protobuf version updated correctly.The version header has been properly updated to reflect the new protobuf compiler version 6.31.1.
124-143: LGTM! Internal protobuf metadata optimizations.The replacement of
~0uplaceholders with explicit bitmap and hasbit index values improves the efficiency of the generated protobuf code. These are internal optimizations that don't affect the external API.
152-154: LGTM! Migration schema simplified appropriately.The migration schema entries have been streamlined to include only the essential offset and size information, reducing memory overhead.
421-422: LGTM! Weak linking attribute added for optimization.The
PROTOBUF_ATTRIBUTE_WEAKattribute enables weak linking for better symbol management in the protobuf runtime.
748-749: LGTM! Weak linking attribute added consistently.Consistent application of
PROTOBUF_ATTRIBUTE_WEAKacross allGetClassData()methods.
1126-1127: LGTM! Weak linking attribute applied consistently.All message class
GetClassData()methods now have the weak linking attribute for improved symbol management.
627-627: LGTM! Improved namespace qualification.The explicit global namespace qualification (
::std::swap) is more precise than relying onusingdeclarations.
641-641: LGTM! Consistent namespace qualification.Consistent use of global namespace qualification for standard library types.
858-878: LGTM! String reference types properly qualified.The explicit global namespace qualification for
::std::stringreferences in serialization methods improves code clarity and avoids potential namespace conflicts.
1010-1010: LGTM! Swap function properly qualified.Consistent application of global namespace qualification for
::std::swap.
1029-1029: LGTM! HasBits type properly qualified.Proper global namespace qualification for standard library types used in internal declarations.
1218-1218: LGTM! String reference properly qualified.Consistent global namespace qualification for string references in CommandResponse serialization.
1308-1308: LGTM! Final swap function properly qualified.Complete and consistent application of global namespace qualification throughout the file.
deps/protobuf/src/google/protobuf/compiler/cpp/padding_optimizer.h (3)
16-17: LGTM! Necessary includes added for refactored design.The new includes for
options.handdescriptor.hare required to support the constructor parameter and virtual method overrides.
29-33: LGTM! Well-designed inheritance-based refactoring.The inheritance from
MessageLayoutHelperpromotes code reuse and thefinaldeclaration is appropriate for this concrete implementation. The explicit constructor properly initializes the base class with the required descriptor.
36-47: LGTM! Appropriate virtual method implementations.The override methods provide sensible default behavior:
HasProfiledData()correctly indicates no profiling dataGetFieldHotness()assumes all fields are hot, which is reasonable for padding optimizationSingleFieldGroup()implements the basic field grouping strategyThese implementations align well with the padding optimization use case.
agents/grpc/src/proto/info.pb.cc (11)
4-4: Auto-generated version bump
The Protobuf C++ version comment has been updated to 6.31.1, aligning with the dependency upgrade.
135-187: Update internal offsets table for protobuf layout metadata
The explicit bitmap (0x081) and hasbit index offsets have been inserted for all message types, replacing placeholder entries. These changes are part of the internal protobuf layout optimization in v6.31.1.
189-194: Simplify MigrationSchema entries
The migration schema definitions now only include the start offset and size for each message type, removing redundant parameters. This matches the new code‐generation style.
286-291: Add PROTOBUF_ATTRIBUTE_WEAK to GetClassData() for map entry type
TheGetClassData()method ofInfoBody_VersionsEntry_DoNotUseis now markedPROTOBUF_ATTRIBUTE_WEAKto control symbol visibility.
497-502: Add PROTOBUF_ATTRIBUTE_WEAK to GetClassData() for InfoBody
TheGetClassData()method ofInfoBodyis now weakly attributed, aligning it with other generated message types.
706-714: Qualifystd::stringwith global namespace in serialization loops
All occurrences ofstd::stringin the internal serialization code have been updated to::std::stringfor explicit global namespace qualification.Also applies to: 716-724, 734-742, 744-752, 754-762, 764-772, 774-782, 784-792, 803-811
338-342: Qualifystd::declvalwith global namespace in internal type alias
TheHasBitsalias now uses::std::declvalto explicitly refer to the global namespace.
1166-1170: Use global::std::swapin InternalSwap of InfoBody
TheInternalSwapmethod now pulls in::std::swapexplicitly, avoiding ambiguity with any otherswapoverloads.
1196-1202: Qualifystd::declvalwith global namespace in InfoEvent internal type alias
TheHasBitsalias forInfoEventnow uses::std::declval, matching the change inInfoBody.
1311-1316: Add PROTOBUF_ATTRIBUTE_WEAK to GetClassData() for InfoEvent
InfoEvent::GetClassData()has been markedPROTOBUF_ATTRIBUTE_WEAK, consistent with other message types.
1493-1496: Use global::std::swapin InternalSwap of InfoEvent
TheInternalSwapimplementation now explicitly uses::std::swapto avoid potential ADL issues.agents/grpc/src/proto/startup_times.pb.cc (7)
4-4: Auto-generated version bump
The Protobuf C++ version comment has been updated to 6.31.1, in line with the updated dependency.
77-94: Update internal offsets table for protobuf layout metadata
Explicit bitmap entries (0x081) and hasbit index offsets (5) have replaced previous placeholder values in the offsets array for both map entry and message types.
96-100: Simplify MigrationSchema entries
Migration schema definitions now list only the offset and size for each type, removing extra metadata parameters.
184-189: Add PROTOBUF_ATTRIBUTE_WEAK to GetClassData() for map entry type
TheGetClassData()method onStartupTimesEvent_BodyEntry_DoNotUsegains thePROTOBUF_ATTRIBUTE_WEAKattribute for symbol visibility control.
235-242: Qualifystd::declvalwith global namespace in internal type alias
TheHasBitsalias atStartupTimesEvent::_Internalnow uses::std::declvalexplicitly.
359-364: Add PROTOBUF_ATTRIBUTE_WEAK to GetClassData() for StartupTimesEvent
StartupTimesEvent::GetClassData()is now weakly attributed, consistent with other generated types.
556-562: Use global::std::swapin InternalSwap of StartupTimesEvent
Internal swap logic now explicitly imports::std::swapto ensure the intended overload is used.agents/grpc/src/proto/source_code.pb.h (1)
14-19: Runtime / headers version guard will hard–fail the build if the runtime isn’t upgraded
#if PROTOBUF_VERSION != 6031001means every translation-unit that includes this header now requires the 6.31.1 runtime headers and link-time runtime.
Make sure your build containers (and any downstream vendored copies) are bumped in lock-step, otherwise you’ll hit a hard compile error or, worse, an ABI mismatch at runtime.agents/grpc/src/proto/common.pb.cc (2)
279-285:PROTOBUF_ATTRIBUTE_WEAKmay break on MSVC / static buildsThe new weak-symbol annotation on
GetClassData()relies on tool-chain support for ELF weak linkage.
On Windows (MSVC //MDd) this turns into a regular symbol which can yield duplicate-definition link errors when the header is included from multiple DLLs.
If you ship Windows builds, verify that:
- The compiler recognises
__declspec(selectany)as the fallback emitted by protobuf.- No duplicate symbol warnings appear when linking multiple
.ccfiles that include generated headers.
993-1007: Namespace-qualified::std::stringis fine – just an FYIThe serializer now uses a fully-qualified
::std::string& _s, preventing accidentalstdhijacking via ADL.
No functional impact, good defensive change.deps/protobuf/src/google/protobuf/compiler/cpp/extension.cc (1)
17-24: New Abseil / feature-resolver headers add link-time deps – update BUILD files
absl/status/statusor.h,absl/strings/*, andgoogle/protobuf/feature_resolver.hpull in additional static libraries (absl_strings,absl_status,protobuf_feature_resolver, …).Ensure:
deps/{BUILD,Makefile,package.json,…)lists the new libs.- CI images have them installed; otherwise you’ll see undefined-reference errors during link.
agents/grpc/src/proto/exit.pb.cc (12)
4-4: Protobuf compiler version update
The version banner has been updated to 6.31.1, matching the bump in the PR.
121-137: Update message hasbit metadata in offset table
The explicit bitmap entries and hasbit index offsets inTableStruct_exit_2eproto::offsetshave been refreshed for the new schema layout.
148-150: Refresh migration schemas sizes
Theschemasarray entries have been adjusted to reflect the correct byte sizes forError,ExitBody, andExitEvent.
193-195: Fully qualify decltype for has_bits alias
TheHasBitsalias now uses::std::declval, improving clarity and avoiding unqualified lookup.
292-297: Apply weak attribute to GetClassData
GetClassData()is now marked withPROTOBUF_ATTRIBUTE_WEAKfor correct linkage semantics.
483-491: Namespace qualification for swap in InternalSwap
TheInternalSwapmethod now explicitly usesusing ::std::swap;, ensuring the global swap is selected.
498-503: Fully qualify decltype for has_bits alias in ExitBody
Consistent update ofHasBitsalias to::std::declval, matching the other message classes.
608-613: Apply weak attribute to ExitBody::GetClassData
GetClassData()inExitBodynow carriesPROTOBUF_ATTRIBUTE_WEAKas expected.
826-831: Namespace qualification for swap in ExitBody::InternalSwap
Swap invocation is now usingusing ::std::swap;for consistency.
846-852: Fully qualify decltype for has_bits alias in ExitEvent
UpdatedHasBitsalias to use::std::declval, aligning with other classes.
961-965: Apply weak attribute to ExitEvent::GetClassData
GetClassData()forExitEventis now correctly annotated withPROTOBUF_ATTRIBUTE_WEAK.
1143-1147: Namespace qualification for swap in ExitEvent::InternalSwap
TheInternalSwapimplementation now imports::std::swapexplicitly.agents/grpc/src/proto/source_code.pb.cc (11)
4-4: Protobuf compiler version update
The version comment has been bumped to 6.31.1, consistent with the PR’s dependency update.
98-119: Update message hasbit metadata in offset table
The offsets forSourceCodeArgsandSourceCodeEventnow include explicit bitmap entries and updated hasbit index offsets.
122-126: Refresh migration schemas sizes
Migration schema entries have been updated to reflect the correct struct sizes for both message types.
167-171: Fully qualify decltype for has_bits alias in SourceCodeArgs
TheHasBitsalias now uses::std::declval, improving namespace clarity.
276-281: Apply weak attribute to SourceCodeArgs::GetClassData
AddedPROTOBUF_ATTRIBUTE_WEAKtoGetClassData()for proper linkage.
386-392: Fully qualify std::string in SourceCodeArgs serialization
String references in_InternalSerializeare now explicitlyconst ::std::string&, removing any ambiguity.
488-493: Namespace qualification for swap in SourceCodeArgs::InternalSwap
using ::std::swap;has been added to ensure the correct swap is used.
509-515: Fully qualify decltype for has_bits alias in SourceCodeEvent
Consistent use of::std::declvalfor theHasBitsalias inSourceCodeEvent.
633-638: Apply weak attribute to SourceCodeEvent::GetClassData
GetClassData()is now marked withPROTOBUF_ATTRIBUTE_WEAK, matching other messages.
768-783: Fully qualify std::string in SourceCodeEvent serialization
Bothpathandcodefields now useconst ::std::string&in_InternalSerialize.
918-923: Namespace qualification for swap in SourceCodeEvent::InternalSwap
Explicitusing ::std::swap;added to the swap implementation.agents/grpc/src/proto/blocked_loop.pb.cc (7)
4-4: Version update correctly reflects the protobuf upgrade.The protobuf compiler version comment has been properly updated to 6.31.1 as expected for this dependency upgrade.
179-227: Internal offset table optimizations are correctly applied.The replacement of placeholder values (~0u) with explicit bitmap (0x081) and hasbit index offset values represents internal metadata improvements in the protobuf 6.31.1 code generator. These changes optimize field access patterns and are consistent across all message types.
236-240: Schema migration optimization correctly implemented.The schema entries have been simplified to use starting offsets and message sizes, removing explicit field range indices. This internal optimization improves the efficiency of the generated protobuf code.
295-295: Namespace qualification improvements enhance code robustness.The change from
std::declvalto::std::declvalprovides full qualification of the standard namespace, which is a best practice in generated code to prevent potential namespace conflicts.
534-534: String reference namespace qualification correctly applied.The change to
const ::std::string&ensures full qualification of the standard string type, maintaining consistency with other namespace improvements.
712-712: Swap function namespace qualification is appropriate.The change to
using ::std::swap;provides explicit global namespace qualification for the swap function, which is important for generated code reliability.
406-407: GetClassData() method formatting and attributes improved.The method signature has been split across two lines for better readability and marked with
PROTOBUF_ATTRIBUTE_WEAKfor proper symbol visibility control in the generated code.deps/protobuf/src/google/protobuf/compiler/cpp/BUILD.bazel (7)
84-84: New field chunking source file properly added.The addition of
field_chunk.ccintroduces new field chunking functionality for the C++ protobuf compiler, enhancing code generation optimization capabilities.
96-96: Message layout helper source file correctly included.The addition of
message_layout_helper.ccprovides new message layout optimization functionality, replacing or enhancing the previous padding optimizer approach.
106-106: Field chunk header file properly declared.The
field_chunk.hheader is correctly added to the hdrs section, making the field chunking functionality available to other components.
150-150: StatusOr dependency appropriately added.The addition of
@abseil-cpp//absl/status:statusordependency enables proper error handling with StatusOr types in the new field chunking and message layout functionality.
162-167: Test proto files properly reorganized.The splitting of test files into
test_bad_identifiers_editions.protoandtest_bad_identifiers_proto2.protowith the addition ofcpp_features_protodependency improves test coverage for different protobuf features and editions.
273-287: Field chunk test target properly configured.The new
field_chunk_testtarget is correctly structured with appropriate dependencies for testing the field chunking functionality, including necessary protobuf libraries, test frameworks, and Abseil components.
449-450: Test proto sources filegroup correctly updated.The filegroup properly reflects the split of test proto files into separate editions and proto2 variants, maintaining consistency with the proto_library target changes.
agents/grpc/src/proto/profile.pb.cc (14)
4-4: Version bump to Protobuf C++ 6.31.1
This reflects the intended upgrade from 6.30.2 to 6.31.1 in the generated sources.
161-161: Generated offsets array update
This explicit bitmap value replaces the previous placeholder. No manual edits needed.
176-183: Replaced placeholder~0uentries with explicit layout values
These changes are entirely in generated table metadata.
188-190: Updated bitmap and hasbit index for HeapProfileArgs/HeapSamplingArgs
Pure generated code adjustments.
264-264: Qualifiedstd::declvalwith global namespace
Using::std::declvalis correct for full namespace qualification.
500-501: AddedPROTOBUF_ATTRIBUTE_WEAKtoGetClassData()
MarkingGetClassData()as weak is consistent with the new compiler output.
831-831: Swapped inusing ::std::swap;inInternalSwap
Qualifyingswapin the global namespace improves ADL correctness.
913-914: AddedPROTOBUF_ATTRIBUTE_WEAKto CPUProfileArgsGetClassData()
Consistent with other message types in this generation.
1047-1048: AddedPROTOBUF_ATTRIBUTE_WEAKto HeapProfileArgsGetClassData()
Matches the new protoc-generated pattern.
1217-1217: Swapped inusing ::std::swap;in HeapProfileArgsInternalSwap
Aligns with upstream namespace qualification.
1322-1323: AddedPROTOBUF_ATTRIBUTE_WEAKto HeapSamplingArgsGetClassData()
Follows the standard generated annotations.
1525-1525: Swapped inusing ::std::swap;in HeapSamplingArgsInternalSwap
Ensures proper ADL behavior.
1625-1626: AddedPROTOBUF_ATTRIBUTE_WEAKto HeapSnapshotArgsGetClassData()
Consistent with other message implementations post-upgrade.
1764-1764: Swapped inusing ::std::swap;in HeapSnapshotArgsInternalSwap
Matches the updated global swap qualification.deps/protobuf/src/google/protobuf/any_lite.cc (6)
20-21: Includeport_def.incat top of file
Managing platform-specific macro definitions before pointer annotations.
52-54: AnnotatedInternalPackFromLitepointers for nullability
AddingPROTOBUF_NONNULLclarifies non-null contracts fordst_urlanddst_value.
61-61: Annotateddst_messageinInternalUnpackToLiteas non-null
Improves static analysis guarantees.
72-74: AnnotatedParseAnyTypeUrlparameters for nullability
url_prefixnullable andfull_type_namenon-null pointers are now explicit.
86-87: OverloadParseAnyTypeUrlannotated as non-null
Clarifies thatfull_type_namemust not be null.
95-95: Includeport_undef.incat end of file
Restores platform macros after pointer annotations.deps/protobuf/src/google/protobuf/any.cc (4)
26-27: AnnotatedInternalPackFrompointers for nullability
dst_urlanddst_valuemarkedPROTOBUF_NONNULLto enforce non-null usage.
32-34: Second overload ofInternalPackFromannotated
Ensuresdst_urlanddst_valuepointers are non-null across all overloads.
39-42: AnnotatedInternalUnpackTodestination message as non-null
AddingPROTOBUF_NONNULLtomessageparameter for clarity.
44-47: AnnotatedGetAnyFieldDescriptorspointers for nullability
type_url_fieldandvalue_fieldparameters now express their nullability contracts.deps/protobuf/src/google/protobuf/api.pb.cc (5)
4-4: LGTM: Protobuf version upgrade reflected correctly.The version comment update from 6.30.2 to 6.31.1 correctly reflects the protobuf dependency upgrade.
140-142: LGTM: Internal metadata structure updates.The changes to bitmap and hasbit index offset values in the offsets table are part of the protobuf compiler's internal metadata optimization. These changes improve field layout efficiency without affecting the public API.
Also applies to: 157-159, 174-176
185-187: LGTM: Migration schema simplification.The simplification of migration schema entries by removing redundant parameters and specifying only start offset and size improves code generation efficiency.
390-391: LGTM: Weak linkage attributes added.The addition of
PROTOBUF_ATTRIBUTE_WEAKto GetClassData() methods enables better symbol resolution across compilation units and aligns with protobuf's improved linking strategy.Also applies to: 889-890, 1356-1357
245-245: LGTM: Global namespace qualifications added.The systematic addition of global namespace qualifications (changing
std::to::std::) improves code robustness by avoiding potential name lookup issues and ensures consistent symbol resolution.Also applies to: 522-522, 554-554, 731-731, 757-757, 1020-1020, 1030-1030, 1049-1049, 1233-1233, 1258-1258, 1444-1444, 1454-1454, 1548-1548
deps/protobuf/src/google/protobuf/arena.cc (2)
125-128: LGTM: Nullability annotations improve static analysis.The addition of
PROTOBUF_NONNULLannotations to theChunkList::AddFallbackmethod parameters clarifies the non-null pointer contracts and enhances static analysis capabilities without changing functionality.
987-999: LGTM: Arena allocation methods properly annotated.The
PROTOBUF_NONNULLannotations on Arena's allocation methods (Allocate,AllocateForArray,AllocateAlignedWithCleanup) correctly document the non-null return value contracts and parameter expectations, improving code clarity and static analysis.deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.h (4)
17-17: LGTM: Span header added for improved parameter passing.The inclusion of
absl/types/span.hsupports the transition from vector parameters to span parameters for better efficiency.
35-36: LGTM: Improved parameter passing efficiency.Changing from
std::vector<int>toabsl::Span<const int>forhas_bit_indicesandinlined_string_indicesparameters eliminates unnecessary copies and provides a more flexible interface that accepts various container types.
41-46: LGTM: Well-designed static factory method.The new
BuildFieldOptionsstatic method properly encapsulates the logic for constructing field options from message descriptors and related data. The method signature is well-designed with appropriate const-correctness and span parameters.
55-55: LGTM: Test access properly granted.Adding
TailCallTableInfoTestas a friend class enables proper unit testing of private members while maintaining encapsulation for production code.deps/protobuf/src/google/protobuf/compiler/cpp/field_chunk.h (6)
1-12: LGTM: Proper header structure and includes.The header guards, includes, and port_def.inc inclusion follow protobuf conventions correctly. The minimal set of includes avoids unnecessary dependencies.
19-30: LGTM: Well-designed FieldChunk structure.The
FieldChunkstruct provides a clean abstraction for grouping fields with shared attributes. The boolean flags (has_hasbit,is_rarely_present,should_split) clearly represent field characteristics relevant to C++ code generation.
34-39: LGTM: Proper function declarations with export macros.The
GenChunkMaskfunction overloads are properly declared withPROTOC_EXPORTfor symbol visibility and take appropriate const-reference parameters to avoid unnecessary copies.
43-57: LGTM: Flexible template-based field collection.The
CollectFieldstemplate function provides a clean, predicate-based approach to partitioning fields into chunks. The use ofequivalentpredicate makes the function highly reusable for different chunking strategies.
59-69: LGTM: Efficient chunk iteration utility.The
FindNextUnequalChunktemplate function provides an efficient way to find the next chunk that differs from the current one, enabling optimized chunk processing algorithms.
73-91: LGTM: Useful field extraction with predicate filtering.The
ExtractFieldstemplate function efficiently extracts and removes fields matching a predicate from a range of chunks. The use ofstd::remove_iffor in-place filtering is efficient and appropriate.deps/protobuf/src/google/protobuf/compiler/cpp/message.h (3)
80-82: LGTM!The updated comment clearly documents the behavioral change -
GenerateOffsetsnow only returns the count of entries rather than handling bitmap-based offset emission internally. This simplification aligns with the refactored layout optimization approach.
204-209: Good use of template parameters with backward compatibility.The addition of default template parameter
kIsV2 = falsemaintains backward compatibility while allowing the code to differentiate between V1 and V2 byte size calculation paths. This is a clean approach to introducing new functionality.
78-78:✅ Verification successful
Breaking change: Verify all callers of GenerateSchema have been updated.
The removal of the
has_offsetparameter fromGenerateSchemais a breaking API change. This could cause compilation errors in any code that still passes this parameter.Run the following script to verify all callers have been updated:
🏁 Script executed:
#!/bin/bash # Description: Search for calls to GenerateSchema to ensure they match the new signature # Search for calls to GenerateSchema method ast-grep --pattern 'GenerateSchema($$$)' -l cpp cc # Also search with ripgrep for more context rg -A 3 -B 3 '->GenerateSchema\(' --type cpp rg -A 3 -B 3 '\.GenerateSchema\(' --type cppLength of output: 601
The previous search didn’t actually scan any source files. Let’s rerun using ripgrep across all C++ sources:
🏁 Script executed:
#!/bin/bash set -e # Search for any calls to GenerateSchema in .cc, .cpp, and .h files rg -n --glob '*.cc' --glob '*.cpp' --glob '*.h' 'GenerateSchema\s*\(' || true rg -n --glob '*.cc' --glob '*.cpp' --glob '*.h' '->GenerateSchema\(' || true rg -n --glob '*.cc' --glob '*.cpp' --glob '*.h' '\.GenerateSchema\(' || trueLength of output: 1011
GenerateSchema signature updated consistently – no internal callers remain using the old signature
I ran a full code-wide search across.h,.cc, and.cppfiles and confirmed:
- Declaration in
deps/protobuf/src/google/protobuf/compiler/cpp/message.h:78now usesGenerateSchema(io::Printer* p, int offset)- Definition in
deps/protobuf/src/google/protobuf/compiler/cpp/message.cc:2344matches the new signature- All internal calls (e.g.
deps/protobuf/src/google/protobuf/compiler/cpp/file.cc:1187) have been updated accordinglyNo remaining references to the old signature were found.
deps/protobuf/src/google/protobuf/arena_cleanup.h (3)
32-54: Good addition of nullability annotations for static analysis.The addition of
PROTOBUF_NONNULLannotations to the destructor function parameters and CleanupNode members helps catch potential null pointer dereferences at compile time. This improves code safety without runtime overhead.
62-86: Consistent nullability annotations throughout the API.The non-null annotations on the
Add,AddFallback, andAddFromExistingmethods correctly document that these methods expect valid pointers. This helps prevent misuse of the API.
93-98: Appropriate use of nullable annotations for optional pointers.The
PROTOBUF_NULLABLEannotations on the member pointers correctly indicate that these can be null, particularly in the initial state. This is important documentation for maintainers.deps/protobuf/src/file_lists.cmake (3)
1308-1308: Test infrastructure updates for better organization.The replacement of
map_test_util_impl.hwithmap_test_util.handtest_util.incwithtest_util2.hsuggests improvements in test organization. The cleaner naming is appreciated.Also applies to: 1334-1334
1571-1572: Proto test files split for better separation of concerns.Splitting
test_bad_identifiers.protointo edition-specific and proto2-specific files (test_bad_identifiers_editions.protoandtest_bad_identifiers_proto2.proto) provides better test isolation and clarity.
490-497: UPB generator file reorganization.The addition of several UPB generator files (
common.cc,file_layout.cc,minitable/fasttable.cc, etc.) with their corresponding headers indicates significant updates to the UPB code generation infrastructure.Also applies to: 633-641
deps/protobuf/src/google/protobuf/any.pb.h (4)
4-4: LGTM! Version update is consistent.The protobuf version update from 6.30.2 to 6.31.1 is properly reflected here.
15-19: LGTM! Version check macro properly updated.The runtime version check is correctly updated to 6031001 matching the protobuf version 6.31.1.
90-90: LGTM! Consistent namespace qualification for std::move.The change to use
::std::moveis consistent with the pattern of explicit global namespace qualification used throughout this header.
279-290: LGTM! Consistent namespace qualification for std::string.All occurrences of
std::stringhave been systematically changed to::std::stringto use explicit global namespace qualification. This is a consistent pattern throughout the protobuf-generated code and helps avoid potential namespace conflicts.Also applies to: 293-304, 365-475
deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.cc (8)
23-23: LGTM! Added necessary include for absl::Span.The include for
absl/types/span.his properly added to support the refactoring fromstd::vectortoabsl::Span.
54-68: LGTM! Constructor properly refactored to use absl::Span.The constructor now accepts
absl::Span<const int>parameters and correctly copies theinlined_string_indicesspan into a vector member. This improves efficiency by avoiding unnecessary vector copies.
71-84: LGTM! Field options construction properly extracted.The field options construction logic has been properly extracted into the static
BuildFieldOptionsmethod, improving code organization and testability. The initialization oftc_table_info_is cleaner and more maintainable.
86-115: Well-structured static method for building field options.The new
BuildFieldOptionsstatic method:
- Properly uses
absl::Spanfor efficient parameter passing- Maintains the same logic as before but in a more reusable form
- Correctly reserves capacity before populating the vector
- Handles all field option configurations consistently
140-141: LGTM! Function signatures updated to use absl::Span.The
MakeNumToEntryTablefunction declaration is properly updated to acceptabsl::Span<const FieldDescriptor* const>.
142-146: LGTM! FieldNameDataSize updated to use absl::Span.The function properly uses
absl::Span<const uint8_t>which is more flexible than vector and avoids unnecessary copies.
195-196: LGTM! MakeNumToEntryTable implementation updated.The function implementation is correctly updated to match the declaration using
absl::Span.
426-429:❓ Verification inconclusive
Important: Verify compatibility with enum range consumers.
The field names in enum range auxiliary entries have been changed from
start/sizetofirst/last. This appears to be a breaking change in the generated code structure that could affect code consuming these auxiliary entries.Run the following script to verify that all consumers of enum range auxiliary entries have been updated:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old field names in enum range handling # Search for references to enum_range.start or enum_range.size in the codebase echo "Searching for old enum_range field references..." rg -A 3 -B 3 'enum_range\.(start|size)' --type cpp # Search for TailCallTableInfo::kEnumRange usage to ensure compatibility echo -e "\nSearching for kEnumRange usage patterns..." rg -A 5 -B 5 'kEnumRange' --type cpp # Check the definition of enum_range struct to confirm the field rename echo -e "\nChecking enum_range struct definition..." ast-grep --pattern 'struct $_EnumRange { $$$ }'Length of output: 30480
Would you please confirm the definition of the auxiliary‐entry struct and its
enum_rangemember? Specifically, let’s verify the exact names of its fields and their types. For example, inspectAuxEntryinparse_function_generator.ccand the definition of theEnumRangetype (andTailCallTableInfoif relevant):
🏁 Script executed:
#!/bin/bash # Show AuxEntry definition grep -R -n -C3 "struct AuxEntry" deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.cc # Show enum_range declaration within AuxEntry grep -R -n -C2 "enum_range" deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.cc # Locate the EnumRange type definition grep -R -n -C3 "EnumRange" deps/protobuf/src/google/protobuf/compiler # Locate TailCallTableInfo definition for context grep -R -n -C5 "struct TailCallTableInfo" deps/protobuf/src/google/protobuf/compiler/cppLength of output: 3488
I want to inspect the actual definitions of both
AuxEntryandTailCallTableInfo(to see howenum_rangeis declared). Please run:
🏁 Script executed:
#!/bin/bash # 1. Show where AuxEntry is defined in the generator rg -n "AuxEntry" -C5 deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.cc # 2. Find the header defining TailCallTableInfo rg -l "class TailCallTableInfo" -tcpp deps/protobuf/src/google/protobuf/compiler | while read -r file; do echo "=== $file ===" rg -n -C5 "enum_range" "$file" doneLength of output: 478
Could you please manually verify the actual definitions of the auxiliary‐entry and its enum_range member to ensure they’ve been renamed as expected? In particular:
- Open
deps/protobuf/src/google/protobuf/compiler/cpp/parse_function_generator.h
– Locatestruct AuxEntryand confirm thatenum_rangehas fields namedfirstandlast(notstart/size).
– LocateTailCallTableInfo(in the same header or nearby) and confirm the enum case forkEnumRangemaps to thatenum_rangemember.Once you’ve confirmed those definitions, you can be confident all consumers match the new
first/lastnaming.deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.cc (9)
1-31: LGTM! Well-structured includes and utility function.The file has appropriate includes and a well-defined utility function
EndsWithMsgPtrthat checks if the last field is a non-repeated, non-lazy message pointer.
33-48: LGTM! FieldGroup methods are correctly implemented.The
FieldGroupclass methods:
EstimateMemorySize()correctly sums up alignment sizesAppend()properly delegates to the update method- Comparison operator provides consistent ordering based on preferred location
49-64: Well-implemented field merging with weighted average calculation.The
UpdatePreferredLocationAndInsertOtherFieldsmethod correctly:
- Handles empty field groups
- Calculates weighted average for preferred location
- Appends fields from the other group
The weighted average calculation ensures that field groups maintain a reasonable preferred location when merged.
66-76: LGTM! Clear optimization pipeline.The
DoOptimizeLayoutmethod implements a clear three-stage optimization:
- Build alignment groups
- Merge groups to reduce padding
- Order fields for optimal layout
78-104: Well-designed field classification methods.The classification methods provide a good foundation for layout optimization:
GetFieldFamilycategorizes fields into logical groupsGetFieldHotnessCategoryprioritizes fields by usage patterns- Proper handling of split fields and special cases
139-168: Excellent padding optimization through alignment consolidation.The
MergeFieldAlignmentGroupsmethod implements a sophisticated approach to minimize padding:
- Groups 1-byte fields into 4-byte blocks
- Groups 4-byte blocks into 8-byte blocks
- Maintains field families and hotness categories throughout
This hierarchical grouping effectively reduces memory waste from padding.
170-240: Sophisticated field ordering with profile-driven optimization.The
BuildFieldDescriptorOrdermethod implements an advanced ordering strategy:
- Uses profiled data when available for optimal hot path performance
- Handles incomplete blocks intelligently to minimize padding
- Switches between recipes based on field characteristics
- Maintains stable ordering for consistency
The logic for placing incomplete 4-byte blocks is particularly clever, ensuring they pair up to form complete 8-byte blocks when possible.
242-273: Well-implemented field consolidation with proper bounds checking.The
ConsolidateAlignedFieldGroupsmethod:
- Correctly validates alignment relationships
- Efficiently groups fields to reach target alignment
- Uses stable sort to ensure consistent output
- Properly reserves capacity for performance
195-202: 🛠️ Refactor suggestionAdd validation for incomplete block assumption.
The code assumes there's at most one incomplete 4-byte block and it's always the last element. This assumption should be validated more explicitly.
Add validation to ensure the assumption holds:
auto it = absl::c_find_if(partition, [](const FieldGroup& fg) { return fg.EstimateMemorySize() <= 4; }); if (it != partition.end()) { // There should be at most one incomplete 4-byte block, and it will // always be the last element. ABSL_CHECK(it + 1 == partition.end()); + // Verify no other incomplete blocks exist + ABSL_CHECK(absl::c_count_if(partition, [](const FieldGroup& fg) { + return fg.EstimateMemorySize() <= 4; + }) == 1) << "Multiple incomplete 4-byte blocks found";Likely an incorrect or invalid review comment.
deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_editions.proto (4)
44-44: Good test coverage for C++ string type features.The proto effectively tests various C++ string types including STRING_PIECE, CORD, and VIEW, which is important for validating the code generator's handling of different string representations.
Also applies to: 70-70, 74-74, 124-124, 126-126
86-108: Comprehensive testing of integral type name conflicts.Excellent coverage of potential naming conflicts with C++ integral types (uint8, int32_t, size_t, etc.). This ensures the code generator properly handles fields with names that match common C++ type names.
206-209: LGTM! Extension range with verification option.The extension range declaration properly uses the edition syntax with verification = UNVERIFIED.
26-27:✅ Verification successful
Verify the necessity of the generic services option.
The comment indicates this option was "auto-added" and references an internal Google link. Since this is for open-source protobuf, ensure this option is actually needed for the test cases.
🏁 Script executed:
#!/bin/bash # Check if cc_generic_services is used elsewhere in the test files rg "cc_generic_services" --type proto -A 2 -B 2Length of output: 77
I’ll search across all .proto files without relying on an undefined file type:
🏁 Script executed:
#!/bin/bash # Search for any occurrences of cc_generic_services in .proto files rg "cc_generic_services" -n --glob '*.proto'Length of output: 429
To confirm whether these tests define any services (and thus need the generic services option), let’s inspect their contents:
🏁 Script executed:
#!/bin/bash # Print the first 80 lines to see service definitions and the cc_generic_services option echo "=== test_bad_identifiers_proto2.proto ===" sed -n '1,80p' deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_proto2.proto echo echo "=== test_bad_identifiers_editions.proto ===" sed -n '1,80p' deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_editions.protoLength of output: 4661
Checking for any service definitions in these test files:
🏁 Script executed:
#!/bin/bash # Look for 'service' declarations in the two test files rg "^\s*service " -n deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_proto2.proto deps/protobuf/src/google/protobuf/compiler/cpp/test_bad_identifiers_editions.protoLength of output: 432
Option cc_generic_services Is Required
Both
test_bad_identifiers_proto2.protoandtest_bad_identifiers_editions.protodeclare aservice TestConflictingMethodNames, so thecc_generic_services = trueoption is necessary to enable generic-service codegen in these tests. No change needed.deps/protobuf/src/google/protobuf/compiler/cpp/message_layout_helper.h (3)
30-69: Well-designed FieldGroup class with clear purpose.The
FieldGroupclass provides a clean abstraction for grouping fields with similar characteristics. TheEstimateMemorySize()method and its documentation clearly explain the memory layout optimization strategy.
139-146: Well-documented field family categories.The FieldFamily enum provides clear categorization of fields based on their initialization and destruction patterns, which is crucial for optimizing memory layout and generated code performance.
158-164: Pure virtual methods provide good extensibility.The pure virtual methods (
HasProfiledData,GetFieldHotness,SingleFieldGroup) allow concrete implementations to customize the layout optimization based on profile data or other criteria.deps/protobuf/src/google/protobuf/compiler/cpp/field_generators/string_view_field.cc (4)
26-28: Proper inclusion of platform-specific headers.Good practice to include
port_def.incandport_undef.incto manage platform-specific macro definitions consistently across the codebase.
217-217: Consistent namespace qualification throughout.Excellent work standardizing all namespace qualifications with
::std::and::absl::prefixes. This prevents potential naming conflicts and improves code clarity.Also applies to: 239-241, 302-302, 320-320, 325-325, 508-508, 562-563, 652-658, 662-663, 671-701, 705-706, 712-713, 723-724, 728-729, 734-735, 742-743, 747-748
673-678: Good use of conditional compilation for bytes fields.The
bytes_taglambda elegantly handles the different treatment needed for BYTES vs STRING fields without duplicating code.
791-792: LGTM! Proper header guard management.Correctly includes
port_undef.incat the end to clean up any macros defined byport_def.inc.deps/protobuf/src/google/protobuf/any.h (2)
49-56: Adds explicit nullability contracts – good for static analysisThe introduction of
PROTOBUF_NONNULLondst_url,dst_value, anddst_messagemakes the expectations explicit and will help clang-tidy / MSVC SAL catch misuse at call-sites.
No issues spotted.
136-139: Double-pointer nullability annotation might be ambiguous
const FieldDescriptor* PROTOBUF_NULLABLE* PROTOBUF_NONNULL value_fieldmixes a non-null outer pointer with a potentially null inner pointer.
Some static-analysis tools interpret this combination differently (e.g. they treat it as “outer pointer is non-null, inner must be null”, instead of “may be null”).
Please confirm that the expansion inport_def.incproduces the intended attributes for all supported compilers (Clang ≥16, MSVC 17, GCC 12).deps/protobuf/src/google/protobuf/BUILD.bazel (2)
552-566:micro_stringintroduces a third string implementation – check for ABI clashesThe new
micro_stringlibrary brings its own small-string optimisation on top ofabsl::Cord/std::string.
Make sure:
- It is only linked into a single shared object to avoid ODR violations.
- Down-stream code does not accidentally mix
micro_stringwithstd::stringAPIs via implicit conversions.If your Bazel build also produces a static
libprotobuf.a, confirm that no duplicate symbols such asgoogle::protobuf::internal::MicroString::kInlineSizeare emitted.
674-681:protobuf_litenow depends onmicro_string– verify transitive consumers
micro_stringis added as an unconditional dependency. Any target that previously relied on the lean symbol surface ofprotobuf_litewill now pull this extra code in.Consider adding a
select()guard (e.g.//build_defs:enable_micro_string) if-and-when mobile or embedded targets want to stay on a minimal footprint.deps/protobuf/src/google/protobuf/compiler/cpp/message.cc (1)
521-543: Possible logic gap when skipping empty chunks
is_same_haswordnow treats chunks withfields.empty()as sharing the same has-word and the innerforloop breaks after pushing the first non-empty chunk mask.
If two or more consecutive empty chunks precede a non-empty one, the iterator advancement logic (for (; it != next; ++it)) will skip those empty chunks silently, but the outerwhile (it != end)later setsit = next– effectively moving past them twice.
The resulting pointer arithmetic is safe (it never crossesend), yet the second advance means at least one real chunk is never processed.Double-check iteration intent and consider handling empty chunks explicitly before
FindNextUnequalChunk().deps/protobuf/src/google/protobuf/arena.h (9)
80-82: Consistent nullability for internal utilitiesThe
arena_delete_objectenforces a non-null object, whileCanUseInternalSwapandCanMoveWithInternalSwapcorrectly accept nullableArena*pointers and only return true when both arenas match.Also applies to: 84-93, 95-104
125-126: Accurate nullability in ArenaOptions fieldsIn
ArenaOptions,initial_block,block_alloc, andblock_deallocare properly marked as nullable, and the function-pointer return/parameter types remain non-null, matching the intended allocation/deallocation contracts.Also applies to: 135-140
175-177: Nullable initial_block in Arena constructorThe overload
Arena(char* PROTOBUF_NULLABLE initial_block, size_t)cleanly signals thatinitial_blockmay be null without changing runtime behavior.
199-200: Factory and destruction APIs nullability
Create,Destroy, andCreateArraynow uniformly mark thearenaparameter as nullable and ensure returned or passed-in object pointers are non-null, aligning with Protobuf’s allocation guarantees.Also applies to: 233-234, 259-260
350-353: InternalHelper swap and retrieval annotations
InternalSwapenforces non-null objects, andGetArenaaccepts a non-null object while returning a nullable arena pointer. The annotations accurately reflect usage.Also applies to: 354-356
401-403: Static SFINAE utilities nullabilityThe SFINAE helpers (
DestructorSkippable,Construct, andNew) now clearly annotate their parameters and return values for nullability, improving static analysis without affecting behavior.Also applies to: 424-427, 429-431
495-497: Cleanup and compatibility methods nullability
ReturnArrayMemoryand bothCreateArenaCompatibleoverloads correctly annotate non-null object pointers and nullable arenas, matching the intended memory-management protocol.Also applies to: 499-501, 515-516
529-532: Allocation routines enforce non-null resultsAll low-level allocation APIs (
AllocateInternal,AllocateAlignedForArray,Allocate,AllocateForArray, andAllocateAlignedWithCleanup) now promise non-null returns, aligning with the existing inline macros and expectations.Also applies to: 594-600, 608-612
645-651: Default/CopyConstruct and string specialization nullabilityOut-of-line
DefaultConstruct,CopyConstruct, and thestd::stringspecialization forAllocateInternalcorrectly enforce non-null return pointers and non-null input parameters.Also applies to: 654-656, 668-671
| if (field->is_repeated()) { | ||
| stats.repeated_total_pcount += analysis.presence_count; | ||
| if (analysis.element_stats.has_value()) { | ||
| stats.repeated_elem_stats = *analysis.element_stats; | ||
| } | ||
| } else { | ||
| stats.singular_total_pcount += analysis.presence_count; | ||
| } |
There was a problem hiding this comment.
Element-stats are overwritten, leading to double-counting
Here the struct’s repeated_elem_stats is assigned, while a few lines later it is added (+=).
The first assignment wipes any previous accumulation, so every repeated field is effectively counted twice (once by = and once by +=) and all prior fields are lost.
- if (analysis.element_stats.has_value()) {
- stats.repeated_elem_stats = *analysis.element_stats;
- }
+ if (analysis.element_stats.has_value()) {
+ stats.repeated_elem_stats += *analysis.element_stats;
+ }Also delete the later += to avoid a second accumulation:
- if (field->is_repeated() && analysis.element_stats.has_value()) {
- ABSL_LOG(INFO) << "Repeated field: " << field->full_name()
- << " has element stats: " << analysis.element_stats->mean;
- stats.repeated_elem_stats += *analysis.element_stats;
- }
+ if (field->is_repeated() && analysis.element_stats.has_value()) {
+ ABSL_LOG(INFO) << "Repeated field: " << field->full_name()
+ << " has element stats: " << analysis.element_stats->mean;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (field->is_repeated()) { | |
| stats.repeated_total_pcount += analysis.presence_count; | |
| if (analysis.element_stats.has_value()) { | |
| stats.repeated_elem_stats = *analysis.element_stats; | |
| } | |
| } else { | |
| stats.singular_total_pcount += analysis.presence_count; | |
| } | |
| if (field->is_repeated()) { | |
| stats.repeated_total_pcount += analysis.presence_count; | |
| if (analysis.element_stats.has_value()) { | |
| stats.repeated_elem_stats += *analysis.element_stats; | |
| } | |
| } else { | |
| stats.singular_total_pcount += analysis.presence_count; | |
| } |
| if (field->is_repeated()) { | |
| stats.repeated_total_pcount += analysis.presence_count; | |
| if (analysis.element_stats.has_value()) { | |
| stats.repeated_elem_stats = *analysis.element_stats; | |
| } | |
| } else { | |
| stats.singular_total_pcount += analysis.presence_count; | |
| } | |
| if (field->is_repeated() && analysis.element_stats.has_value()) { | |
| ABSL_LOG(INFO) << "Repeated field: " << field->full_name() | |
| << " has element stats: " << analysis.element_stats->mean; | |
| } |
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto.cc
around lines 344 to 351, the code assigns repeated_elem_stats with = which
overwrites previous values, and later adds to it with += causing
double-counting. To fix this, remove the assignment (=) and only accumulate
repeated_elem_stats using += to correctly sum stats without losing prior data or
double-counting.
| #include <unistd.h> | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential portability issue with POSIX-specific header.
The inclusion of unistd.h is POSIX-specific and may cause compilation issues on Windows platforms. Consider adding platform-specific conditional compilation if Windows support is required.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are Windows-specific build configurations or CMake files that might indicate Windows support requirements
# Check for Windows-specific build files
fd -t f "CMakeLists.txt|BUILD|WORKSPACE|\.vcxproj|\.sln" | head -10
# Look for platform-specific code patterns
rg -i "windows|win32|_win|ifdef.*windows" --type cpp | head -10Length of output: 1082
🏁 Script executed:
#!/bin/bash
# Locate the analyze_profile_proto_main.cc file and inspect its includes and unistd.h usage
file_path=$(fd -t f "analyze_profile_proto_main.cc" | head -n1)
echo "File path: $file_path"
# Show the first 50 lines for context
sed -n '1,50p' "$file_path"
echo
# Search the file for usages of unistd.h functions/macros
rg -n "unistd.h" -n "$file_path"
rg -n -e "getpid|getuid|getopt|sleep|unlink|access|isatty" "$file_path" || echo "No other unistd.h-specific calls found."Length of output: 2795
Remove unused <unistd.h> include for portability
No POSIX functions from <unistd.h> are used in this file, and keeping it will break Windows builds. Please remove the include to restore cross-platform compatibility.
• File: deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto_main.cc:16
-#include <unistd.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <unistd.h> |
🤖 Prompt for AI Agents
In
deps/protobuf/src/google/protobuf/compiler/cpp/tools/analyze_profile_proto_main.cc
at line 16, the #include <unistd.h> directive is present but no POSIX-specific
functions from this header are used in the file. To improve portability and
avoid breaking Windows builds, remove the #include <unistd.h> line entirely
since it is unused and platform-specific.
| #include "google/protobuf/compiler/cpp/test_bad_identifiers_editions.pb.h" | ||
| #include "google/protobuf/compiler/cpp/test_bad_identifiers_proto2.pb.h" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the split test header files exist.
The replacement of a single test_bad_identifiers.pb.h with separate test_bad_identifiers_editions.pb.h and test_bad_identifiers_proto2.pb.h files aligns with protobuf's editions feature. This separation allows testing of both legacy proto2 and new editions syntax.
🏁 Script executed:
#!/bin/bash
# Description: Verify the new test header files exist in the protobuf source
# Expected: Both header files should be present
echo "Checking for test_bad_identifiers_editions.pb.h:"
fd "test_bad_identifiers_editions.pb.h" deps/protobuf/
echo "Checking for test_bad_identifiers_proto2.pb.h:"
fd "test_bad_identifiers_proto2.pb.h" deps/protobuf/
echo "Verifying old file is removed or still exists:"
fd "test_bad_identifiers.pb.h" deps/protobuf/Length of output: 466
Missing split test header files
The includes for
#include "google/protobuf/compiler/cpp/test_bad_identifiers_editions.pb.h"
#include "google/protobuf/compiler/cpp/test_bad_identifiers_proto2.pb.h"in deps/protobuf/src/google/protobuf/compiler/cpp/unittest.inc (lines 48–49) refer to files that are not present under deps/protobuf/. Neither the new headers nor the original test_bad_identifiers.pb.h were found in the tree.
Please ensure one of the following:
- Commit or generate
test_bad_identifiers_editions.pb.handtest_bad_identifiers_proto2.pb.hinto the correct path. - If they live elsewhere, update the include paths accordingly.
- Remove or replace these includes if the tests have been reorganized or the old header is no longer used.
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/unittest.inc around lines 48
to 49, the included header files test_bad_identifiers_editions.pb.h and
test_bad_identifiers_proto2.pb.h are missing from the deps/protobuf/ directory.
To fix this, either add or generate these header files in the correct location,
update the include paths if the files exist elsewhere, or remove/replace these
includes if the tests have been moved or the headers are no longer needed.
| namespace { | ||
| bool ShouldGenerateFeatureSetDefaultData(absl::string_view extension) { | ||
| return extension == "pb.java" || extension == "pb.test"; | ||
| } | ||
| } // namespace |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Name-match in ShouldGenerateFeatureSetDefaultData() looks fragile
return extension == "pb.java" || extension == "pb.test";extension receives descriptor_->full_name(), which usually contains the package prefix
(e.g. "pb.pb.java" or "my.pkg.pb.java").
A strict equality check will fail in those cases, and the default-data block will never be emitted.
Consider comparing only the unqualified extension name, e.g.:
-return extension == "pb.java" || extension == "pb.test";
+return absl::EndsWith(extension, "pb.java") ||
+ absl::EndsWith(extension, "pb.test");🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/extension.cc around lines 104
to 108, the function ShouldGenerateFeatureSetDefaultData uses a strict equality
check on the extension string, which includes package prefixes and causes
incorrect results. Modify the function to extract and compare only the
unqualified extension name (the suffix after the last dot) against "pb.java" and
"pb.test" to ensure correct matching regardless of package prefixes.
| void MaybeEmitUpdateCachedHasbits(const FieldDescriptor* field, | ||
| io::Printer* p, | ||
| int& cached_has_word_index) const; | ||
|
|
||
| void EmitUpdateByteSizeV2ForNumerics( | ||
| size_t field_size, io::Printer* p, int& cached_has_word_index, | ||
| std::vector<const FieldDescriptor*>&& fields) const; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Ensure proper test coverage for the new V2 helper methods.
The new helper methods MaybeEmitUpdateCachedHasbits and EmitUpdateByteSizeV2ForNumerics are specific to the V2 implementation. These need comprehensive testing to ensure correctness.
Run the following script to check for test coverage:
🏁 Script executed:
#!/bin/bash
# Description: Search for tests related to the new V2 methods
# Search for test files that might test these methods
rg -l "ByteSizeV2|MaybeEmitUpdateCachedHasbits|EmitUpdateByteSizeV2ForNumerics" --type cpp | grep -i test
# Look for V2-specific test cases
rg "TEST.*ByteSize.*V2" --type cppLength of output: 144
Add C++ unit tests for the new V2 helper methods
No existing tests reference MaybeEmitUpdateCachedHasbits or EmitUpdateByteSizeV2ForNumerics. Please add coverage in the C++ test suite, for example:
- In
google/protobuf/compiler/cpp/message_unittest.cc(or a new*_unittest.cc), write tests for:MaybeEmitUpdateCachedHasbitsverifying that has-bit words are updated correctly.EmitUpdateByteSizeV2ForNumericscovering various numeric field sizes and combinations.
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/message.h around lines 211 to
217, the new V2 helper methods MaybeEmitUpdateCachedHasbits and
EmitUpdateByteSizeV2ForNumerics lack unit test coverage. Add C++ unit tests for
these methods in google/protobuf/compiler/cpp/message_unittest.cc or a new
unittest file. Write tests that verify MaybeEmitUpdateCachedHasbits correctly
updates has-bit words and that EmitUpdateByteSizeV2ForNumerics handles various
numeric field sizes and combinations properly.
| ${protobuf_SOURCE_DIR}/src/google/protobuf/compiler/cpp/enum.cc | ||
| ${protobuf_SOURCE_DIR}/src/google/protobuf/compiler/cpp/extension.cc | ||
| ${protobuf_SOURCE_DIR}/src/google/protobuf/compiler/cpp/field.cc | ||
| ${protobuf_SOURCE_DIR}/src/google/protobuf/compiler/cpp/field_chunk.cc |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Message layout optimization refactoring looks good.
The replacement of padding_optimizer.cc with field_chunk.cc and message_layout_helper.cc indicates a significant refactoring of the layout optimization logic. The modular approach with separate field chunking and layout helper components is cleaner.
Also applies to: 379-379, 508-508, 515-515
🏁 Script executed:
#!/bin/bash
# Description: Verify the removal of padding_optimizer and its replacement
# Confirm padding_optimizer.cc is removed
fd padding_optimizer.cc
# Check if there are any remaining references to the old padding optimizer
rg -i "padding.?optimizer" --type cpp --type h | grep -v "padding_optimizer.h"Length of output: 239
Remove leftover PaddingOptimizer reference in message.cc
A reference to the removed PaddingOptimizer class remains in message.cc, which will break the build. Please update it to use the new MessageLayoutHelper API.
• File: deps/protobuf/src/google/protobuf/compiler/cpp/message.cc
- Replace the instantiation:
- message_layout_helper_ = std::make_unique<PaddingOptimizer>(descriptor); + message_layout_helper_ = std::make_unique<MessageLayoutHelper>(descriptor);
- Update the include at the top of the file:
- #include "padding_optimizer.h" + #include "message_layout_helper.h"
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/message.cc around the lines
referencing PaddingOptimizer, remove all references to the old PaddingOptimizer
class and replace them with the new MessageLayoutHelper API. Update the include
directives at the top of the file to remove padding_optimizer.h and add
message_layout_helper.h instead. Modify the instantiation and usage of the
layout optimizer to use MessageLayoutHelper methods and constructors accordingly
to ensure the build does not break.
| ${protobuf_SOURCE_DIR}/src/google/protobuf/generated_message_util.cc | ||
| ${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.cc | ||
| ${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.cc | ||
| ${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.cc |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New internal feature helper added to core protobuf.
The addition of internal_feature_helper.cc to the core protobuf sources suggests new internal functionality for feature management. Ensure this is properly documented and tested.
Also applies to: 72-72
🏁 Script executed:
#!/bin/bash
# Description: Check the implementation and documentation of internal_feature_helper
# Look at the header file for API documentation
cat src/google/protobuf/internal_feature_helper.h | head -50
# Check if there are corresponding tests
fd -e cc -e cpp internal_feature_helper_testLength of output: 187
🏁 Script executed:
#!/bin/bash
# Locate the header file for internal_feature_helper and show its first 50 lines
header_path=$(fd internal_feature_helper.h)
echo "Header found at: $header_path"
head -n 50 "$header_path"
# Search for any test files referencing internal_feature_helper
echo "Searching for tests..."
rg -n 'internal_feature_helper' --glob '*_test.*'Length of output: 2345
Add Unit Tests for InternalFeatureHelper
The header already contains inline documentation, but I didn’t find any existing tests covering internal_feature_helper. Please add unit tests to validate its APIs and guard against regressions.
• deps/protobuf/src/google/protobuf/internal_feature_helper.h
– Documentation is present in the header.
• No test files reference InternalFeatureHelper (verified with rg 'internal_feature_helper' --glob '*_test.*').
Please add a new test (e.g., internal_feature_helper_test.cc) under your test suite to cover both public and friend-only methods.
🤖 Prompt for AI Agents
In deps/protobuf/src/file_lists.cmake at line 49, the new
internal_feature_helper.cc file has been added but there are no existing unit
tests for InternalFeatureHelper. To fix this, create a new test file named
internal_feature_helper_test.cc in the appropriate test directory. Write
comprehensive unit tests covering both the public API and any friend-only
methods of InternalFeatureHelper to ensure proper validation and prevent
regressions. Also verify that the tests are included in the build system.
| int num_generated_indices = 1; | ||
| const auto make_bitmap = [&](auto... bits) { | ||
| uint32_t res = 0; | ||
| int index = 0; | ||
| ((res |= (static_cast<uint32_t>(bits) << index++)), ...); | ||
| ((num_generated_indices += bits), ...); | ||
| return absl::StrCat("0x", absl::Hex(res, absl::kZeroPad3)); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Undefined evaluation order in make_bitmap lambda
The fold expression mutates and reads index (index++) inside the same
unsequenced expression:
((res |= (static_cast<uint32_t>(bits) << index++)), ...);The increment side-effect is unsequenced relative to other pack expansions
(pre-C++20) and results in undefined behaviour on most compilers.
Refactor to a simple loop:
-auto make_bitmap = [&](auto... bits) {
- uint32_t res = 0;
- int index = 0;
- ((res |= (static_cast<uint32_t>(bits) << index++)), ...);
- ((num_generated_indices += bits), ...);
- return absl::StrCat("0x", absl::Hex(res, absl::kZeroPad3));
-};
+auto make_bitmap = [&](auto... bits) {
+ uint32_t res = 0;
+ int idx = 0;
+ for (bool b : {bits...}) {
+ if (b) res |= (1u << idx);
+ num_generated_indices += b;
+ ++idx;
+ }
+ return absl::StrCat("0x", absl::Hex(res, absl::kZeroPad3));
+};This is well-defined and still constexpr-friendly.
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/message.cc around lines 2595
to 2603, the fold expression in the make_bitmap lambda uses index++ inside an
unsequenced pack expansion, causing undefined behavior. Refactor the lambda to
replace the fold expression with a simple loop that iterates over the bits,
shifting and accumulating into res while incrementing index in a well-defined
sequence. This will ensure defined evaluation order and maintain
constexpr-friendliness.
| void MessageGenerator::MaybeEmitUpdateCachedHasbits( | ||
| const FieldDescriptor* field, io::Printer* p, | ||
| int& cached_has_word_index) const { | ||
| 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"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cached has-bits update references generated symbol literally
this_.$has_bits$[$index$] is emitted verbatim; $has_bits$ is only defined
by surrounding WithVars(...) when a corresponding substitution is active.
If the caller forgets to wrap the emitter with HasBitVars, the placeholder
will leak into generated C++ and fail to compile.
Wrap the whole Emit in the same var-context used elsewhere or emit the
concrete member name (_impl_._has_bits_) directly.
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/message.cc around lines 1332
to 1346, the code emits the string this_.$has_bits$[$index$] literally without
ensuring the $has_bits$ variable is defined, causing compilation errors if the
caller does not provide the substitution. To fix this, either wrap the Emit call
in the appropriate variable context that defines $has_bits$ (such as using
WithVars or HasBitVars) or replace the placeholder with the actual member name
like _impl_._has_bits_ directly in the emitted string to avoid relying on
external substitutions.
| template <bool kIsV2> | ||
| void MessageGenerator::EmitCheckAndUpdateByteSizeForField( | ||
| const FieldDescriptor* field, io::Printer* p) const { | ||
| absl::AnyInvocable<void()> emit_body = [&] { | ||
| field_generators_.get(field).GenerateByteSize(p); | ||
| const auto& gen = field_generators_.get(field); | ||
| if constexpr (!kIsV2) { | ||
| gen.GenerateByteSize(p); | ||
| } else { | ||
| } | ||
| }; |
There was a problem hiding this comment.
GenerateByteSizeV2 path left unimplemented
Inside EmitCheckAndUpdateByteSizeForField<kIsV2> the else branch for the V2
calculation is empty (lines 1293-1296).
Any call executed with kIsV2 == true will therefore emit no byte-size logic,
silently under-counting the size and corrupting serialization.
- } else {
- }
+ } else {
+ gen.GenerateByteSizeV2(p); // or appropriate V2 helper
+ }Ensure a full implementation exists before enabling the V2 path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <bool kIsV2> | |
| void MessageGenerator::EmitCheckAndUpdateByteSizeForField( | |
| const FieldDescriptor* field, io::Printer* p) const { | |
| absl::AnyInvocable<void()> emit_body = [&] { | |
| field_generators_.get(field).GenerateByteSize(p); | |
| const auto& gen = field_generators_.get(field); | |
| if constexpr (!kIsV2) { | |
| gen.GenerateByteSize(p); | |
| } else { | |
| } | |
| }; | |
| template <bool kIsV2> | |
| void MessageGenerator::EmitCheckAndUpdateByteSizeForField( | |
| const FieldDescriptor* field, io::Printer* p) const { | |
| absl::AnyInvocable<void()> emit_body = [&] { | |
| const auto& gen = field_generators_.get(field); | |
| if constexpr (!kIsV2) { | |
| gen.GenerateByteSize(p); | |
| } else { | |
| gen.GenerateByteSizeV2(p); // or appropriate V2 helper | |
| } | |
| }; |
🤖 Prompt for AI Agents
In deps/protobuf/src/google/protobuf/compiler/cpp/message.cc around lines 1288
to 1297, the else branch for the kIsV2 == true case in
EmitCheckAndUpdateByteSizeForField is empty, causing no byte-size calculation
for V2 and leading to serialization errors. Implement the V2 byte-size
calculation logic in this else branch, likely by calling the appropriate
GenerateByteSizeV2 method or equivalent on the field generator to correctly
compute the byte size when kIsV2 is true.
santigimeno
force-pushed
the
santi/upgrade_protobuf
branch
from
c820f89 to
cf600b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deps/protobuf/.bazelci/README.md (1)
3-6: Replace bare URL & fix minor grammar for lint-compliance and claritymarkdownlint flags the naked URL, and the sentence has a small subject/verb agreement issue. Updating both improves readability and keeps CI green.
-The `.bazelci/presubmit.yml` file is used by https://buildkite.com/bazel/protobuf -to test building protobuf with Bazel on Bazel CI. It should contain the same -set of tests as `.bcr/presubmit.yml` which is the tests that will run before -publishing protobuf to BCR. +The `.bazelci/presubmit.yml` file is used by +[Buildkite Bazel Protobuf CI](https://buildkite.com/bazel/protobuf) +to test building protobuf with Bazel on Bazel CI. It should contain the same +set of tests as `.bcr/presubmit.yml`, which are the tests that will run before +publishing protobuf to the Bazel Central Registry (BCR).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/logs/v1/logs_service.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/logs/v1/logs_service.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/metrics/v1/metrics_service.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/metrics/v1/metrics_service.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/trace/v1/trace_service.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/collector/trace/v1/trace_service.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/common/v1/common.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/common/v1/common.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/logs/v1/logs.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/logs/v1/logs.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/metrics/v1/metrics.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/metrics/v1/metrics.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/resource/v1/resource.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/resource/v1/resource.pb.his excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/trace/v1/trace.pb.ccis excluded by!**/gen/**deps/opentelemetry-cpp/third_party/opentelemetry-proto/gen/cpp/opentelemetry/proto/trace/v1/trace.pb.his excluded by!**/gen/**
📒 Files selected for processing (37)
agents/grpc/src/proto/asset.pb.cc(7 hunks)agents/grpc/src/proto/asset.pb.h(7 hunks)agents/grpc/src/proto/blocked_loop.pb.cc(23 hunks)agents/grpc/src/proto/blocked_loop.pb.h(14 hunks)agents/grpc/src/proto/command.pb.cc(16 hunks)agents/grpc/src/proto/command.pb.h(19 hunks)agents/grpc/src/proto/common.pb.cc(15 hunks)agents/grpc/src/proto/common.pb.h(16 hunks)agents/grpc/src/proto/exit.pb.cc(15 hunks)agents/grpc/src/proto/exit.pb.h(16 hunks)agents/grpc/src/proto/info.pb.cc(20 hunks)agents/grpc/src/proto/info.pb.h(36 hunks)agents/grpc/src/proto/metrics.pb.cc(8 hunks)agents/grpc/src/proto/metrics.pb.h(4 hunks)agents/grpc/src/proto/nsolid_service.pb.cc(6 hunks)agents/grpc/src/proto/nsolid_service.pb.h(7 hunks)agents/grpc/src/proto/packages.pb.cc(15 hunks)agents/grpc/src/proto/packages.pb.h(20 hunks)agents/grpc/src/proto/profile.pb.cc(16 hunks)agents/grpc/src/proto/profile.pb.h(7 hunks)agents/grpc/src/proto/reconfigure.pb.cc(11 hunks)agents/grpc/src/proto/reconfigure.pb.h(16 hunks)agents/grpc/src/proto/source_code.pb.cc(12 hunks)agents/grpc/src/proto/source_code.pb.h(15 hunks)agents/grpc/src/proto/startup_times.pb.cc(7 hunks)agents/grpc/src/proto/startup_times.pb.h(4 hunks)deps/protobuf/.bazelci/README.md(1 hunks)deps/protobuf/.bazelci/presubmit.yml(1 hunks)deps/protobuf/.bazelrc(1 hunks)deps/protobuf/.bcr/metadata.template.json(0 hunks)deps/protobuf/.bcr/presubmit.yml(2 hunks)deps/protobuf/protobuf.gyp(2 hunks)deps/protobuf/src/file_lists.cmake(26 hunks)deps/protobuf/src/google/protobuf/BUILD.bazel(24 hunks)deps/protobuf/src/google/protobuf/any.cc(1 hunks)deps/protobuf/src/google/protobuf/any.h(3 hunks)deps/protobuf/src/google/protobuf/any.pb.cc(8 hunks)
💤 Files with no reviewable changes (1)
- deps/protobuf/.bcr/metadata.template.json
✅ Files skipped from review due to trivial changes (9)
- deps/protobuf/.bazelrc
- deps/protobuf/.bazelci/presubmit.yml
- agents/grpc/src/proto/startup_times.pb.h
- agents/grpc/src/proto/nsolid_service.pb.h
- agents/grpc/src/proto/blocked_loop.pb.h
- agents/grpc/src/proto/exit.pb.h
- agents/grpc/src/proto/metrics.pb.cc
- agents/grpc/src/proto/common.pb.cc
- agents/grpc/src/proto/blocked_loop.pb.cc
🚧 Files skipped from review as they are similar to previous changes (25)
- deps/protobuf/protobuf.gyp
- deps/protobuf/.bcr/presubmit.yml
- agents/grpc/src/proto/asset.pb.h
- agents/grpc/src/proto/common.pb.h
- agents/grpc/src/proto/profile.pb.h
- agents/grpc/src/proto/packages.pb.h
- agents/grpc/src/proto/nsolid_service.pb.cc
- agents/grpc/src/proto/info.pb.h
- agents/grpc/src/proto/metrics.pb.h
- deps/protobuf/src/google/protobuf/any.pb.cc
- agents/grpc/src/proto/asset.pb.cc
- agents/grpc/src/proto/reconfigure.pb.h
- agents/grpc/src/proto/packages.pb.cc
- agents/grpc/src/proto/info.pb.cc
- agents/grpc/src/proto/startup_times.pb.cc
- agents/grpc/src/proto/source_code.pb.cc
- agents/grpc/src/proto/reconfigure.pb.cc
- agents/grpc/src/proto/profile.pb.cc
- agents/grpc/src/proto/command.pb.cc
- deps/protobuf/src/google/protobuf/any.cc
- agents/grpc/src/proto/exit.pb.cc
- agents/grpc/src/proto/source_code.pb.h
- agents/grpc/src/proto/command.pb.h
- deps/protobuf/src/file_lists.cmake
- deps/protobuf/src/google/protobuf/any.h
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
deps/protobuf/.bazelci/README.md
3-3: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: lint-js-and-md
- GitHub Check: build-tarball
- GitHub Check: coverage-windows
🔇 Additional comments (13)
deps/protobuf/src/google/protobuf/BUILD.bazel (13)
34-37: LGTM: Consistent UPB target additions.The addition of
upb_c_proto_libraryandupb_proto_reflection_librarytargets for all well-known types follows a consistent pattern and properly depends on the corresponding proto libraries. This aligns with the protobuf 31.1 update introducing expanded UPB support.Also applies to: 39-42, 59-62, 64-67, 80-83, 85-88, 101-104, 106-109, 133-136, 138-141, 154-157, 159-162, 175-178, 180-183, 200-203, 205-208, 221-224, 226-229
552-565: LGTM: Well-structured micro_string library addition.The new
micro_stringlibrary and its corresponding test are properly structured with appropriate dependencies. The library depends on arena components and the test includes necessary testing dependencies.Also applies to: 567-583
680-680: LGTM: Appropriate micro_string dependency additions.The addition of
micro_stringas a dependency to bothprotobuf_liteandprotobuftargets is correct and necessary for the new string handling functionality.Also applies to: 777-777
721-721: LGTM: Proper internal_feature_helper integration.The addition of
internal_feature_helper.hto the headers list andinternal_feature_helper.ccto the protobuf target sources is correctly structured for the new internal feature helper component.Also applies to: 758-758
807-807: LGTM: Appropriate charset dependency addition.The addition of
@abseil-cpp//absl/strings:charsetdependency is reasonable for enhanced string processing capabilities in protobuf 31.1.
1347-1347: LGTM: Appropriate test utility header addition.The addition of
map_test_util.hto thelite_test_utiltarget is correctly placed for map-related testing functionality.
1415-1415: LGTM: Improved test_util2 dependency structure.The addition of
:test_utilas a dependency to thetest_util2target establishes a proper dependency relationship, which is better than the previous textual headers approach.
1573-1595: LGTM: Well-structured lazily_build_dependencies_test addition.The new
lazily_build_dependencies_testtarget is properly structured with comprehensive dependencies that are appropriate for testing lazy dependency building functionality. The configuration is consistent with other test targets.
1638-1639: LGTM: Necessary test dependency additions.The addition of dependencies to various test targets is appropriate for the updated functionality:
cpp_features_cc_protoandportfor dynamic_message_unittestlite_test_utilfor generated_message_reflection_unittestcc_lite_test_protosfor lite_arena_unittestportfor unknown_field_set_unittestThese additions ensure tests have access to the required components.
Also applies to: 1645-1646, 1752-1752, 1822-1822, 2217-2217
1709-1738: LGTM: Comprehensive internal_feature_helper_test addition.The new
internal_feature_helper_testtarget is well-structured with appropriate dependencies for testing the internal feature helper functionality. The configuration is consistent with other test targets in the file.
1413-1413: LGTM: Appropriate test exclusion.The exclusion of
lazily_build_dependencies_test.ccfrom thefull_test_srcsfilegroup is consistent with other specialized tests that are excluded from the general test suite.
1421-1425: LGTM: Organized test source filegroup addition.The new
lazily_build_dependencies_test_srcsfilegroup provides proper organization for the specialized test source file, consistent with the pattern used for other specialized test sources.
1-2449: LGTM: Comprehensive and well-structured protobuf 31.1 update.The changes in this BUILD.bazel file are consistent with the protobuf 31.1 update and include:
- New UPB targets: Added
upb_c_proto_libraryandupb_proto_reflection_librarytargets for all well-known types- New micro_string library: Added with proper dependencies and test coverage
- New internal_feature_helper: Added to headers and sources with corresponding test
- Updated dependencies: Added necessary dependencies to existing targets
- New test targets: Added comprehensive test coverage for new functionality
- Proper organization: New filegroups and exclusions maintain clean structure
All changes follow established patterns and maintain consistency with the existing codebase structure. The dependency relationships are appropriate and the new functionality is properly tested.
santigimeno
force-pushed
the
santi/upgrade_protobuf
branch
from
cf600b0 to
c951f2c
Compare
PR-URL: #325 Reviewed-By: Rafael Gonzaga <[email protected]>
|
Landed in 3a53c65 |
PR-URL: #325 Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #325 Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #325 Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #359 Reviewed-By: Juan José Arboleda <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style
Tests
Chores