You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/09/11 15:49:05 UTC
[GitHub] [incubator-tvm] rkimball opened a new pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
rkimball opened a new pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456
With an eye towards cleaning up Windows warnings we can start by enabling more clang warnings since clang gives more expressive messages.
This PR enables -Weverything, which enables all available warning messages in clang and then whitelisting those warnings that are too pedantic. Some of the warnings should be addressed in the future but enabling them now would make the PR too large with fixes. We can deal with enabling more warnings one at a time in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691333600
For users of TVM I would suggest we turn off warnings. There is no reason for a user to see warnings if they can't really do anything about them. The warnings should only be for TVM develops.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] antinucleon commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
antinucleon commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691315061
+1 to @junrushao1994 's solution.
This PR works, but it is close to write a Makefile rather than a CMake solution.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691312663
Hey @tmoreau89 @rkimball, I just came up with an idea!
To be more consistent with the cmake-way, it might be have a separate file, like "cmake/util/WarningFlags.cmake", which provides a variable ${TVM_WARNING_FLAGS} if this file is included.
Inside this file, we check the compiler version (GCC, Clang, MSVC, etc), and for each compiler version, we may have a list of candidate warning flags to be added. Then we iterator through the list and check if this flag exists. If it does, then we add it to the result ${TVM_WARNING_FLAGS}.
Then, from the outside users' perspective, it is a cross-platform way to provide a list of warning flags, which is very cmake :-)
BTW, personally I don't prefer using "-Weverything" plus canceling some flags, but would be great to choose the combination of specific flags instead, which might make the list clearer.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691174008
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r488103706
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf section 17.6.4.3.2
For global names:
Each name that contains a double underscore _ _ or begins with an underscore followed by an uppercase letter (2.12) is reserved to the implementation for any use.
I never can remember these rules so to simplify things I never start any name with underscore and never use double underscores. There are cases where it is fine but I find it too difficult for me to remember when.
This define is within a namespace so I think it might be ok, but why tempt fate?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tmoreau89 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691299758
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
Also FYI https://github.com/apache/incubator-tvm/pull/6450 fixes the MSVC warnings with a shorter list of disabled warnings.
Perhaps we can go with -Wextra -pedenatic for now as per https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-692337935
-Wextra and disabling unused-parameter adds only a single deprected-copy warning that would be easy to fix.
-Wpedantic and disabling extra-semi adds gnu-anonymous-struct and nested-anon-types.
We could just enable individual warnings we are interested in.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r487307521
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691312663
Hey @tmoreau89 @rkimball, I just came up with an idea!
To be more consistent with the cmake-way (cross-platform/compiler with unified API), it might be have a separate file, like "cmake/util/WarningFlags.cmake", which provides a variable ${TVM_WARNING_FLAGS} if this file is included.
Inside this file, we check the compiler version (GCC, Clang, MSVC, etc), and for each compiler version, we may have a list of candidate warning flags to be added. Then we iterator through the list and check if this flag exists. If it does, then we add it to the result ${TVM_WARNING_FLAGS}.
Then, from the outside users' perspective, it is a cross-platform way to provide a list of warning flags, which is very cmake :-)
BTW, personally I don't prefer using "-Weverything" plus canceling some flags, but would be great to choose the combination of specific flags instead, which might make the list clearer.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691335753
@tqchen I like the idea of periodically turning on -Weverything manually. It is easy to do and we can see where we stand and if we want to add any new flags.
I will remove -Weverything and move the greatly reduced list of of warnings we want to enable over time to a separate "flags" cmake file.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-696462772
Thanks @rkimball @tmoreau89 @junrushao1994 !
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r488820408
##########
File path: CMakeLists.txt
##########
@@ -144,7 +144,7 @@ else(MSVC)
CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0)
set(CMAKE_CXX_FLAGS "-faligned-new ${CMAKE_CXX_FLAGS}")
endif()
- include(cmake/clang_flags.cmake)
+ include(cmake/modules/clang_flags.cmake)
Review comment:
Please use ClangFlags.cmake (CamelCase) to be consistent with the rest of the files under modules
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-696462772
Thanks @rkimball @tmoreau89 @junrushao1994 !
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691333984
I feel like if a warning is good, we should turn it on, enforce it in our CI treat warning as error.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691333020
@rkimball how about we follow the recommendation to only turn on -Weverything locally to see what is going on, but keep something more cross platform like `- Wextra, -Wall, -pedantic`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
I was wondering if the change is specific to a certain version of clang compiler? Does those flags exist in other compilers? If so, do we have a more "cmake" way to control which flags we are using?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691174008
@tqchen
@tmoreau89
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691174008
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] antinucleon commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
antinucleon commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691315061
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r487311828
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r487307521
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r487311828
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r487307521
##########
File path: src/tir/transforms/lower_custom_datatypes.cc
##########
@@ -97,7 +97,7 @@ class CustomDatatypesLowerer : public StmtExprMutator {
return expr;
}
-#define DEFINE_MUTATE__(OP, NodeName) \
+#define DEFINE_MUTATE(OP, NodeName) \
Review comment:
What is the warning does this macro leads to? Seems the original code is fine as it is(with underscores as part of the macro)
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
Perhaps underscore in the macro is fine. unless there is a explict reason not to do so
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for the object, because they are created by make_object, and the destructor is stored there.
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
##########
File path: include/tvm/auto_scheduler/cost_model.h
##########
@@ -70,6 +70,11 @@ class CostModelNode : public Object {
LOG(FATAL) << "Not implemented";
}
+ /*!
+ * \brief Default virtual destructor
+ */
+ virtual ~CostModelNode() {}
Review comment:
The default v-destructor is not necessary for a sub-class of object, because they are created by make_object, and the destructor is stored there. https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/memory.h#L74
The `Wdelete-non-virtual-dtor` already covers that case and will create a warning if an object is created using other methods (e.g. new) but deleted via virtual destructor. Right now the code base is free of that warning.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691332209
@junrushao1994 I like the idea for adding the WarningFlags.cmake file. I had it that way in a different project and it worked well there so I will move the flags there.
The error flags are specific to clang and they do add new flags with new releases of clang which is why I like the -Weverything approach. This way if there is a new warning flag added we will know because we will see the new warnings. That's the theory anyway.
I'm looking for an exhaustive list of warning messages to see how many there are.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691333020
@rkimball how about we follow the recommendation to turn on -Weverything locally to see what is going on, but keep something more cross platform like `- Wextra, -Wall, -pedantic`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
Also FYI https://github.com/apache/incubator-tvm/pull/6450 fixes the MSVC warnings with a shorter list of disabled ones.
Perhaps we can go with -Wextra -pedenatic for now as per https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r488306641
##########
File path: cmake/clang_flags.cmake
##########
@@ -0,0 +1,87 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# If we are running clang >= 10.0 then enable more checking. Some of these warnings may not exist
+# in older versions of clang so we limit the use of older clang for these checks.
+if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
Review comment:
Move to cmake/modules/ClangWarnings.cmake, we only keep config.cmake under the cmake folder.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691335360
I will try -Wextra and -pedantic to see what happens.
There is already a list of the warnings I think would be "good things" here https://github.com/apache/incubator-tvm/pull/6456/files#diff-af3b638bc2a3e6c650974192a53c7291R182-R193
perhaps we should just remove -Weverything and then we can just add that list one at a time and fix the warnings, that is if we agree that the warnings are valid. The documentation warnings are great, some of the others are less great but would be good to address.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691333169
@tqchen I agree that -Weverything is not good for building except when we are developing. I could set things up where we need to define a cmake flag like -DTVM_PEDANTIC_WARNINGS or something like that to enable the extra checks. There are some really good warnings in there and I would be fine with just enabling a subset
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-696462772
Thanks @rkimball @tmoreau89 @junrushao1994 !
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tmoreau89 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691300180
@rkimball looks like we're hitting some linting problems in CI, could you address those? thanks
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tmoreau89 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691299758
@junrushao1994 to take a look if you've got cycles
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691174008
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691334017
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on a change in pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
rkimball commented on a change in pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#discussion_r487987645
##########
File path: include/tvm/node/container.h
##########
@@ -1105,7 +1105,7 @@ class DenseMapNode : public MapNode {
friend class MapNode;
};
-#define _TVM_DISPATCH_MAP(base, var, body) \
Review comment:
I found this in the gnu docs here http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
`all identifiers regardless of use that begin with either two underscores or an underscore followed by a capital letter are reserved names`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691334017
One thing we can do is `-Wextra, -pendenatic` then add a list of the warnings you want to enable in the WarningFlags.cmake.
From time to time we can turn on -Weverything locally and see if there is anything that can be added to the list.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691331275
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 edited a comment on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
I am in favor of more warnings, which helps us catch more potential issues as possible.
I was wondering if the change is specific to a certain version of clang compiler? Does those flags exist in other compilers? If so, do we have a more "cmake" way to control which flags we are using?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] antinucleon commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
antinucleon commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691315061
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] tmoreau89 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691299758
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #6456: Enable more warnings when compiling with clang 10.0 or greater
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6456:
URL: https://github.com/apache/incubator-tvm/pull/6456#issuecomment-691306382
----------------------------------------------------------------
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:
users@infra.apache.org