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

[Wsign-compare] Close Wno-error=sign_compare #47163

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

Li-fAngyU
Copy link
Contributor

@Li-fAngyU Li-fAngyU commented Oct 19, 2022

PR types

Others

PR changes

Others

Describe

Close Wno-error=sign_compare

@paddle-bot
Copy link

paddle-bot bot commented Oct 19, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Oct 19, 2022
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1
Copy link
Contributor

luotao1 commented Oct 20, 2022

我下载了这个PR流水线的PR-CI-Coverage和PR-CI-Build。Coverage的编译会多一个deprecated-declaration的warning,可以考虑后续也去掉。

$ grep warning: coverage.log |grep "\[\-W" |grep -v party | awk '{print $NF}'|sort|uniq -c|sort -nr
  21 [-Wdeprecated-declarations]
   5 [-Wterminate]
   4 [-Wunused-variable]
   4 [-Wunused-local-typedefs]
   2 [-Wunknown-pragmas]
$ grep warning: build.log |grep "\[\-W" |grep -v party | awk '{print $NF}'|sort|uniq -c|sort -nr
   6 [-Wterminate]
   4 [-Wunused-variable]
   4 [-Wunused-local-typedefs]
   2 [-Wunknown-pragmas]

@luotao1 luotao1 merged commit 2246e88 into PaddlePaddle:develop Oct 20, 2022
@luotao1
Copy link
Contributor

luotao1 commented Oct 20, 2022

后续可以在PR描述里都加上tracking issue

@@ -36,7 +36,7 @@ endif()
proto_library(ps_framework_proto SRCS the_one_ps.proto)

set(DISTRIBUTE_COMPILE_FLAGS
"-Wno-error=unused-value -Wno-non-virtual-dtor -Wno-error=non-virtual-dtor -Wno-error=delete-non-virtual-dtor -Wno-error=sign-compare -Wno-error=unused-variable -Wno-error=return-type -Wno-error=unused-but-set-variable -Wno-error=unknown-pragmas -Wno-error=parentheses -Wno-error=unused-result"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fluid/distributed 下的CMakeLists里的Wno-error=sign-compare没有去除掉。
去除掉的话会触发第三方库的sign-compare error,报错信息

Copy link
Contributor

Choose a reason for hiding this comment

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

报错信息:

/workspace/Paddle/paddle/fluid/distributed/test/brpc_utils_test.cc:119:3:   required from here
2022-10-19 16:44:33 /workspace/Paddle/build/third_party/install/gtest/include/gtest/gtest.h:1444:11: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
2022-10-19 16:44:33    if (lhs == rhs) {

EXPECT_EQ(slr->rows().size(), 564);

改成 EXPECT_EQ(slr->rows().size(), 564UL); 就不会有这个warning了,可以尝试去掉fluid/distributed 下的CMakeLists里的Wno-error=sign-compare

@Li-fAngyU

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的我修改一下

@Li-fAngyU
Copy link
Contributor Author

我下载了这个PR流水线的PR-CI-Coverage和PR-CI-Build。Coverage的编译会多一个deprecated-declaration的warning,可以考虑后续也去掉。

$ grep warning: coverage.log |grep "\[\-W" |grep -v party | awk '{print $NF}'|sort|uniq -c|sort -nr
  21 [-Wdeprecated-declarations]
   5 [-Wterminate]
   4 [-Wunused-variable]
   4 [-Wunused-local-typedefs]
   2 [-Wunknown-pragmas]
$ grep warning: build.log |grep "\[\-W" |grep -v party | awk '{print $NF}'|sort|uniq -c|sort -nr
   6 [-Wterminate]
   4 [-Wunused-variable]
   4 [-Wunused-local-typedefs]
   2 [-Wunknown-pragmas]

好的

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

Successfully merging this pull request may close these issues.

3 participants