Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build warning, ByteSize() is deprecated, use ByteSizeLong() instead #1723

Merged
merged 4 commits into from
Mar 30, 2022

Conversation

yangzhg
Copy link
Member

@yangzhg yangzhg commented Mar 22, 2022

Fix build warning, protobuf ByteSize() is deprecated, use ByteSizeLong() instead

@yangzhg
Copy link
Member Author

yangzhg commented Mar 23, 2022

build was failed due to https://downloads.apache.org/thrift/0.11.0/thrift-0.11.0.tar.gz has moved to https://archive.apache.org/dist/thrift/0.11.0/thrift-0.11.0.tar.gz

src/brpc/policy/hulu_pbrpc_protocol.cpp Outdated Show resolved Hide resolved
@yangzhg yangzhg requested a review from wwbmmm March 29, 2022 08:00
src/brpc/protocol.h Outdated Show resolved Hide resolved
@serverglen
Copy link
Contributor

LGTM

src/brpc/protocol.h Outdated Show resolved Hide resolved
@wwbmmm wwbmmm merged commit 4839ec2 into apache:master Mar 30, 2022
// use template to avoid include `google/protobuf/message.h`
template<typename T>
inline uint32_t GetProtobufByteSize(const T& message) {
#if GOOGLE_PROTOBUF_VERSION >= 3010000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

在不包含 protobuf 相关头文件的情况下, 直接用 GOOGLE_PROTOBUF_VERSION 有问题吧? 虽然不影响编译, 但结果恐怕不符合预期, 除非源文件中先包含 protobuf 相关头文件再包含此头文件.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前看好像确实都是先包含的 protobuf 相关头文件.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include "brpc/options.pb.h"
这个应该包含了那个宏

Copy link
Member

@wasphin wasphin Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protoc 生成的文件也会定义,把这个文件看漏了…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants