haojin2 commented on a change in pull request #14900: Fix warning / static
function in header.
URL: https://github.com/apache/incubator-mxnet/pull/14900#discussion_r286620518
##########
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:
So if you merge the content of this PR into the other PR, does it not
unblock your other PR? From the title and content of both PRs I'm reading that
you're trying to get rid of a few compilation warnings, so how does this PR not
fit into the scope of that PR then? I'm also a bit confused about how would a
fix for a compilation warning be a dependency of the other PR?
You may also argue that you're addressing different types of warning per PR,
but seems like that PR is a mixed fix for multiple causes of compilation
warnings, so why cannot this fix be a part of it?
To be fair, I'm not against having multiple PRs to complete one goal, but
only if you have a logical split/organization of your PRs. For example, #14926
and #14946 are essentially the same fix, and I personally prefer them to be
merged into the same PR for easier tracking in the future.
Regarding the usage of anonymous namespace, I think keeping the current
convention (simply putting it in the mxnet::op namespace) would cause less
confusion/surprise for other collaborators. Since you believe this is the best
practice, I'd prefer and be supportive of you raising another major refactor PR
in the future to make this happen for every single such cases.
----------------------------------------------------------------
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
