Hi Chris I get the rationale of the point you raise, but In my opinion, and considering the complexity of C++ and the potential for difficult and expensive to track bugs, I think this should be enabled by default for both release and debug. A developer is free to disable warnings in his own private branch, but I don't see what would be the benefit of this.
Regarding your second point, I think this is a minor issue which is outweighed by the benefits. In the case you propose, the author of a PR can easily fix a bunch of warnings when CI fails as usual. For example in case he gets one or two warnings that his version of the compiler didn't catch, or if she has an additional warning of some type with a different version of GCC / Clang. This has the objective to prevent warning inflation. In practice, a different version of GCC might produce just a couple of new warning types that will be easily fixable once we upgrade the compiler in CI. We also get the benefit of preventing warnings on the gcc versions that the author is using, in the case he has a different one. Another option is to enable warnings as errors only on CI. I would prefer to have it enabled by default, for correctness. As first time users are not likely to compile MXNet by themselves, and also considering the significant complexity of compiling MXNet from scratch for newcomers. In general, the compilers that we have running on CI should be our reference compilers. And for practical purposes, having no warnings in those versions of Clang and GCC would be a positive step towards more code quality, clean compilation and a more mantainable code base. Once we have CI stable we can build a matrix of supported compilers in the docs, as for example there are versions of GCC which are not supported by the nvidia tools. Pedro. On Mon, Jan 15, 2018 at 7:27 PM, Chris Olivier <[email protected]> wrote: > If enabled, it should only cause errors in Release builds, since having > warnings in WIP code is not unusual. > > In addition, different developers use different gcc/clang versions. Some > gcc versions, for instance, generate warnings where others do not. It > would not be fair to render unbuildable a developer who is using a newer > (or older) gcc version is different from CI. Can this argument be tied to > a particular compiler/platform/version? > > On Mon, Jan 15, 2018 at 9:43 AM, Marco de Abreu < > [email protected]> wrote: > >> +1 >> >> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy < >> [email protected]> >> wrote: >> >> > Hi >> > >> > I would like to propose to compile in CI with warnings as errors for >> > increased code quality. This has a dual purpose: >> > >> > 1. Enforce a clean compilation output. Warnings often indicate >> > deficiencies in the code and hide new warnings which can be an >> > indicator of problems. >> > >> > 2. Warnings can surface bugs as has happened before. >> > >> > While this might be impractical in all architectures, I would propose >> > having the Linux and Clang build run without warnings in CI. >> > >> > I think we are very close to this as I personally have been fixing >> > warnings in Linux and OSX / Clang. >> > >> > References: >> > >> > https://github.com/apache/incubator-mxnet/pull/9398 >> > >> > http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/ >> > incubator-mxnet/detail/PR-9398/1/pipeline >> > >> > Pedro. >> > >>
