larroy commented on a change in pull request #14900: Fix warning / static
function in header.
URL: https://github.com/apache/incubator-mxnet/pull/14900#discussion_r286717551
##########
File path: src/operator/nn/dropout.cc
##########
@@ -26,6 +26,32 @@
#include "./dropout-inl.h"
#include "../operator_common.h"
+#include "mxnet/op_attr_types.h"
+
+namespace {
+
+using namespace mxnet;
+using namespace mxnet::op;
Review comment:
@haojin2 You make good points and I think there's different schools of
thought on grouping changes in bigger PRs or sending more smaller PRs/commits
that can be reviewed quicker and bisected for bugs easier. While your points
are totally valid, I think solving the warning was a simple change from static
to inline, I separated into a single PR to have it merged quicker than a PR
aggregating other unrelated changes and refactorings that can lead to extended
discussions. In this case you requested additional refactor, I had to spend
additional valuable time dealing with CI failures and again change to the op
namespace which I think is a matter of taste and having diminishing returns. I
would suggest to discuss this issue regarding grouping logical changes in a
bigger PR in the mailing list so there can be a consensus on how we want to do
it in the project. Discussing it on every PR is not productive for external
contributors like me. After your second request for additional refactor I feel
like closing the PR and not fixing any more warnings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
