Fix warnings #14940
Fix warnings #14940
Conversation
|
@mxnet-label-bot add [pr-awaiting-review, Backend] |
marcoabreu
added
Backend
larroy
force-pushed
the
graph_dump
branch
2 times, most recently
from
ffbeab3 to
da53b56
Compare
|
Thanks @larroy for this initiative. We should aim to reduce GCC warning to zero and turn on treat warning as error by default. Do you also plan to send a proposal to the dev list for the GCC warning cleanup, so that future PRs will not add more warnings? |
|
@apeforest I think everybody agreed with that. But somebody has to do it, and I think I'm one of the few moving towards zero warnings. I have spent effort in this ungrateful task, the problem is that there are many platforms and build flavours as you are aware. So it's not easy to pass CI when enabling warnings as errors. Some of my PRs fixing warnings take a long time to merge and create long discussions, maybe you have some ideas on how we could streamline the process or add a tag to the PRs so they can be more expedited when they just fix a warning. |
larroy
changed the title
larroy
force-pushed
the
graph_dump
branch
2 times, most recently
from
8fc19d8 to
705c9d2
Compare
|
@mxnet-label-bot add [pr-work-in-progress] |
This reverts commit 18ae9b6.
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
src/common/exec_utils.cc
Outdated
| return true; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove extra blank lines
| return str; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove extra blank line
There was a problem hiding this comment.
Please address @haojin2 comments before resolving it?
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove extra blank line
There was a problem hiding this comment.
please fix before resolving.
| << oss.str(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove extra blank line
There was a problem hiding this comment.
And at all applicable places.
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
|
Fixed the whitespace, let me know if there's something else. |
|
@haojin2 can you please review again? |
|
@szha this has been opened forever, could you help with this PR? thanks. |
| @@ -0,0 +1,519 @@ | |||
| /* | |||
There was a problem hiding this comment.
I failed to understand what warnings did this fix?
|
It seems moving the inlining functions to a .cc file is not related to fixing warnings (or please correct me if it does). Please move it to a seperate PR if that refactoring is indeed necessary. In fact, I also read this from: https://www.geeksforgeeks.org/inline-functions-cpp/ It seems the compiler will optimize unnecessary inline functions? |
|
So the request for changes is basically removal of two additional blank lines (which by the way are allowed by Google style guide and Pep8) this has a CI cost which I don't find justified for the request of change. I guess I also missed the extra newline when I resolved it, my bad. Moving the functions to the CC file reduces binary sizes before linking. Is there anything seriously wrong with this change that requires two vetos? Also fixing warnings and moving some functions from header to implementation is small change enough that can be merged in a single PR in my opinion. We just had a 6k lines PR merged adding numpy operators, I find that there's two different set of criteria applied to some PRs and not others. This is not really encouraging. |
|
I don't find this way to review code is conductive to contributing to this project. |
|
While I appreciate your desire to be frugal, it really bothers me how often CI costs or runtime are being brought up as reason for not making a change. As long as something is not burning, time is not of essence. CI is provided as a service to the community. Managing costs is not the responsibility of the community but of the people who provide the underlying system. Code in a project might live for a long time. Insisting on high standards is certainly something I'd prioritize above saving a few bucks. Something that certainly remove friction here would be to make our linting rules tighter again. We have a lot of excludes for points that people regularly bring up, so I'd recommend considering removing some of the exceptions. But aside from that, I agree with the other contributors that while it might be easier to manage for an individual to make unrelated changes in the same PR, it causes additional friction due to confusion and mixing concerns. You certainly have good intentions here and I really appreciate that, but these friction points make these reviews difficult. While it's true that small changes are not a problem to merge fast, the combination of multiple small and unrelated changes makes the PR actually big and thus increases the round-trip time as well as required effort to review. |
|
@marcoabreu This is a bit of a rant but why this doesn't apply to huge PRs like this? /~https://github.com/apache/incubator-mxnet/pull/15581/files instead is used as nitpicking for small PRs from external collaborators. If we want PRs which addresses isolated concerns, good, but let's be consistent and not have double standards. My point is that I think is not a good use of my time or the reviewers time to act as a linter. If some committers are so passionate about whitespace they should improve the linter and code analysis tools instead of alienating contributors. I find ok to be reminded of some whitespace issue here and there, but this is a shallow review which comes late. I think comitters should take into consideration volunteers time and effort a bit more instead of making things more difficult. |
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
Description
Fix warnings and move exec_utils implementation of functions to implementation file.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.