Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix warnings in CLang. #15270

Merged
merged 1 commit into from
Jul 19, 2019
Merged

Fix warnings in CLang. #15270

merged 1 commit into from
Jul 19, 2019

Conversation

larroy
Copy link
Contributor

@larroy larroy commented Jun 18, 2019

Description

In file included from ../src/kvstore/kvstore.cc:28:
../src/kvstore/./kvstore_local.h:281:23: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
auto validator = [this](const int key, const NDArray& nd, bool ignore_sparse) -> bool {
^
../src/kvstore/./kvstore_local.h:326:23: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
auto validator = [this](const int key, const RSPVal& val_rowid, bool ignore_sparse) -> bool {
^
In file included from ../src/c_api/c_api_profile.cc:35:
../src/c_api/../profiler/./profiler.h:1160:8: warning: 'mxnet::profiler::ProfileOperator::start' hides overloaded virtual function [-Woverloaded-virtual]
void start(mxnet::Context::DeviceType dev_type, uint32_t dev_id) {
^
../src/c_api/../profiler/./profiler.h:870:8: note: hidden overloaded virtual function 'mxnet::profiler::ProfileEvent::start' declared here: different number of parameters (0 vs 2)
void start() override {
^
../src/c_api/../profiler/./profiler.h:1212:8: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
[this](OprExecStat *stat) {}, name_.c_str(), dev_type_, dev_id_,
^

See also #14940

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@larroy
Copy link
Contributor Author

larroy commented Jun 18, 2019

@mxnet-label-bot add [pr-awaiting-review,build]

@marcoabreu marcoabreu added Build pr-awaiting-review PR is waiting for code review labels Jun 18, 2019
@roywei
Copy link
Member

roywei commented Jul 8, 2019

@larroy could you rebase this PR?

@larroy
Copy link
Contributor Author

larroy commented Jul 10, 2019

@roywei sure.

@larroy
Copy link
Contributor Author

larroy commented Jul 12, 2019

Done, could we get those small PRs that fix warnings merged?

@marcoabreu
Copy link
Contributor

Please enable warnings as error for the ones you have fixed.

@larroy
Copy link
Contributor Author

larroy commented Jul 18, 2019

@marcoabreu I think more warnings got introduced in master, if we make it so tedious to push small fixes, nobody is going to fix warnings. Warnings have to be fixed before enabling all warnings as errors, not before, otherwise some builds will fail. I will enable that once all the PRs fixing warnings are merged, it's impossible otherwise.

In file included from ../src/kvstore/kvstore.cc:28:
../src/kvstore/./kvstore_local.h:281:23: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
    auto validator = [this](const int key, const NDArray& nd, bool ignore_sparse) -> bool {
                      ^
../src/kvstore/./kvstore_local.h:326:23: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
    auto validator = [this](const int key, const RSPVal& val_rowid, bool ignore_sparse) -> bool {
                      ^
In file included from ../src/c_api/c_api_profile.cc:35:
../src/c_api/../profiler/./profiler.h:1160:8: warning: 'mxnet::profiler::ProfileOperator::start' hides overloaded virtual function [-Woverloaded-virtual]
  void start(mxnet::Context::DeviceType dev_type, uint32_t dev_id) {
       ^
../src/c_api/../profiler/./profiler.h:870:8: note: hidden overloaded virtual function 'mxnet::profiler::ProfileEvent::start' declared here: different number of parameters (0 vs 2)
  void start() override {
       ^
../src/c_api/../profiler/./profiler.h:1212:8: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
      [this](OprExecStat *stat) {}, name_.c_str(), dev_type_, dev_id_,
       ^

See also apache#14940
@marcoabreu
Copy link
Contributor

Ok

@marcoabreu
Copy link
Contributor

Btw I'm not talking about enabling all warnings as errors but to enable the ones you're fixing step by step. Otherwise you are going to chase a tail you'll never catch

@larroy
Copy link
Contributor Author

larroy commented Jul 18, 2019

Now CI is failing, I don't think is effective to enable warnings one by one, since there are many compilers, it gets extremely tedious, if you want to do that please go ahead.

@marcoabreu
Copy link
Contributor

As far as I remember, we only have two clang builds. Could you elaborate?

I mean as far as I understand you, you looked at the clang output and fixed a specific category, right? Wouldn't that mean that this specific category is green and can be turned into an error? Sorry if I'm misunderstanding something here.

That's how we did it back then, right @KellenSunderland?

@larroy
Copy link
Contributor Author

larroy commented Jul 18, 2019

I understood but the problem is that there's several compilers that spew different warnings and are compiled with the same flags, I think the best would be to enable all as errors once there's no warnings in any compiler. I think adding flags one by one is too much work.

@larroy
Copy link
Contributor Author

larroy commented Jul 18, 2019

@marcoabreu
Copy link
Contributor

I'm just talking about enabling them in our CI, not globally. Our CI is a pretty controlled environment. Would be a pity if you fix these things and others introduce them.

@larroy
Copy link
Contributor Author

larroy commented Jul 19, 2019

Could you approve? this would help move this effort forward. I will look into enable warnigns as errors as soon as CI passes smoothly in the following PRs.

@marcoabreu marcoabreu merged commit eab6da6 into apache:master Jul 19, 2019
@larroy
Copy link
Contributor Author

larroy commented Jul 19, 2019

appreciate, thanks

Copy link
Contributor

@KellenSunderland KellenSunderland left a comment

Choose a reason for hiding this comment

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

LGTM, good change.

anirudhacharya pushed a commit to anirudhacharya/mxnet that referenced this pull request Aug 20, 2019
In file included from ../src/kvstore/kvstore.cc:28:
../src/kvstore/./kvstore_local.h:281:23: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
    auto validator = [this](const int key, const NDArray& nd, bool ignore_sparse) -> bool {
                      ^
../src/kvstore/./kvstore_local.h:326:23: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
    auto validator = [this](const int key, const RSPVal& val_rowid, bool ignore_sparse) -> bool {
                      ^
In file included from ../src/c_api/c_api_profile.cc:35:
../src/c_api/../profiler/./profiler.h:1160:8: warning: 'mxnet::profiler::ProfileOperator::start' hides overloaded virtual function [-Woverloaded-virtual]
  void start(mxnet::Context::DeviceType dev_type, uint32_t dev_id) {
       ^
../src/c_api/../profiler/./profiler.h:870:8: note: hidden overloaded virtual function 'mxnet::profiler::ProfileEvent::start' declared here: different number of parameters (0 vs 2)
  void start() override {
       ^
../src/c_api/../profiler/./profiler.h:1212:8: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
      [this](OprExecStat *stat) {}, name_.c_str(), dev_type_, dev_id_,
       ^

See also apache#14940
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Build pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants