Re: warnings as errors

2019-05-22 Thread Pedro Larroy
I was not able to fix the warnings on mshadow type switch with unused
local typedefs, that's one example of warning that I would disable. I
couldn't find a way to solve that one and I think the ramifications of
an unused typedef are not likely to cause bugs in the code and are
more of a pedantic nature.

https://github.com/apache/incubator-mxnet/pull/13424

I think turning on them one by one is going to pollute the compilation
output unecesarily and even run into commandline length problems?  I
think is best to enable all warnings and errors and cherry pick the
ones we can't fix or won't fix on purpose.

In this other case, I managed to tighten the warnings but ASAN is
giving some problems:

https://github.com/apache/incubator-mxnet/pull/14850

I think having warning fixes reviewed and merged faster without
triggering additional refactorings could make this process easier,
also having some help in this area and contributions would be greatly
appreciated.

Pedro.

On Tue, May 21, 2019 at 3:49 PM Sheng Zha  wrote:
>
> It would be great to enforce the check for warnings and treat as errors. Some 
> questions I have:
> - what are the warnings that you think should be ignored?
> - for the rest of the warning types, can we turn them on one by one?
>
> -sz
>
> On 2019/05/21 22:33:51, Pedro Larroy  wrote:
> > Hi dev@
> >
> > I try to fix any warning that I see during compilation of MXNet in my
> > platform and with the build toggles that I care about. These seemingly
> > trivial and ungrateful efforts, take nonetheless energy on the
> > contributor side.
> >
> > I think overall I submitted myself more than a dozen of PRs fixing
> > warnings and I would like to call for additional help and
> > contributions in this area.
> >
> > There was a question from Lin about discussing this on the mailing
> > list, I have the feeling that everybody agrees on moving towards zero
> > warnings and warnings as errors. I think there are unavoidable
> > warnings that can be disabled specifically such as the one triggered
> > by mshadow type switch.
> >
> > Some important missing warnings such as warning on missing return
> > values (ie. forgetting to return on a function returning non-void)
> > cause bugs, danger and additional time spent bugfixing, which can be
> > better spent somewhere else.
> >
> > Is there a process that we can figure out such as a more expedited
> > merges of PRs fixing warnings or a specific label?
> >
> > Some simple PRs that fixes a warning can take long to merge, and
> > sometimes trigger too much discussion and make the progress a bit
> > unfriendly to contributors.
> >
> > Any help or constructive ideas on this topic would be appreciated.
> >
> > Pedro.
> >


Re: warnings as errors

2019-05-21 Thread Sheng Zha
It would be great to enforce the check for warnings and treat as errors. Some 
questions I have:
- what are the warnings that you think should be ignored?
- for the rest of the warning types, can we turn them on one by one?

-sz

On 2019/05/21 22:33:51, Pedro Larroy  wrote: 
> Hi dev@
> 
> I try to fix any warning that I see during compilation of MXNet in my
> platform and with the build toggles that I care about. These seemingly
> trivial and ungrateful efforts, take nonetheless energy on the
> contributor side.
> 
> I think overall I submitted myself more than a dozen of PRs fixing
> warnings and I would like to call for additional help and
> contributions in this area.
> 
> There was a question from Lin about discussing this on the mailing
> list, I have the feeling that everybody agrees on moving towards zero
> warnings and warnings as errors. I think there are unavoidable
> warnings that can be disabled specifically such as the one triggered
> by mshadow type switch.
> 
> Some important missing warnings such as warning on missing return
> values (ie. forgetting to return on a function returning non-void)
> cause bugs, danger and additional time spent bugfixing, which can be
> better spent somewhere else.
> 
> Is there a process that we can figure out such as a more expedited
> merges of PRs fixing warnings or a specific label?
> 
> Some simple PRs that fixes a warning can take long to merge, and
> sometimes trigger too much discussion and make the progress a bit
> unfriendly to contributors.
> 
> Any help or constructive ideas on this topic would be appreciated.
> 
> Pedro.
> 


warnings as errors

2019-05-21 Thread Pedro Larroy
Hi dev@

I try to fix any warning that I see during compilation of MXNet in my
platform and with the build toggles that I care about. These seemingly
trivial and ungrateful efforts, take nonetheless energy on the
contributor side.

I think overall I submitted myself more than a dozen of PRs fixing
warnings and I would like to call for additional help and
contributions in this area.

There was a question from Lin about discussing this on the mailing
list, I have the feeling that everybody agrees on moving towards zero
warnings and warnings as errors. I think there are unavoidable
warnings that can be disabled specifically such as the one triggered
by mshadow type switch.

Some important missing warnings such as warning on missing return
values (ie. forgetting to return on a function returning non-void)
cause bugs, danger and additional time spent bugfixing, which can be
better spent somewhere else.

Is there a process that we can figure out such as a more expedited
merges of PRs fixing warnings or a specific label?

Some simple PRs that fixes a warning can take long to merge, and
sometimes trigger too much discussion and make the progress a bit
unfriendly to contributors.

Any help or constructive ideas on this topic would be appreciated.

Pedro.


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Barber, Christopher
I agree, you don’t want to run tests until you feel the code is ready, but 
stopping everything to fix warnings that aren’t likely to cause any actual 
problems just slows down the process. I think that sanity checking makes 
perfect sense, but the usual way to do that is to first run some “smoke” tests 
selected specifically for that purpose.

I also am not advocating for not fixing warnings. I believe you should entirely 
disable all warnings that don’t really matter (if there are any), and fix 
everything else. The existence of warnings in the build should be considered a 
bug, and developers should be strongly encouraged to not check in any code with 
warnings under their build configuration. However, that does not mean you have 
to stop all progress until they are all fixed. It is not like you refuse to 
allow any check-ins until all outstanding bugs are fixed, right?

On 1/16/18, 11:59 AM, "kellen sunderland" <kellen.sunderl...@gmail.com> wrote:

@Christopher: I see your point, but the counter argument would be: "Why
should the project run fairly expensive tests (~20 minutes on few GPU
instances) for code that will require you to amend your commit anyway?"  In
normal circumstances I'd completely agree with you and let the full tests
run for information purposes.  However, given how resource intensive our
testing is I'd prefer that we run some sanity checks first before we launch
a full test run.

@Chris + Pedro: I think it's a good idea to focus discussion on what should
happen in CI.  Most CI builds already have warnings as errors turned on (as
mentioned by Xingjian ) because we have USE_DEV=1 set.  We currently have
the following warnings disabled in CI (i.e. they won't fail the build, and
are not reported): '-Wno-unused-variable -Wno-unused-parameter
-Wno-unknown-pragmas -Wno-unused-local-typedefs'.  I think the next step
here would be attempt to cleanup the warnings that we dislike seeing, and
once we have them cleaned up and merged, start failing those warnings on CI.

I agree with Pedro and Cliff that cleaning these up over time would be a
good demonstration of the boy-scout-principle (
https://martinfowler.com/bliki/OpportunisticRefactoring.html).  In my
experience teams and projects that adhere to this principle end up with
much more readable codebases.

Reference build:

http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/master/runs/245/nodes/54/steps/248/log/?start=0

-Kellen

On Tue, Jan 16, 2018 at 5:52 PM, McCollum, Cliff <mccol...@amazon.co.uk>
wrote:

> While you can debate the "broken windows" theory (
> https://en.wikipedia.org/wiki/Broken_windows_theory) I think it has
> relevance to code warnings: the more warnings you tolerate, the more 
likely
> you are to end up with other undesirable things in your code. My 
preference
> has always been to treat warnings as errors in any code that leaves a
> developer's own machine. If a team/project doesn't do this, it is relying
> on the good intentions of people to prevent code warnings from multiplying
> to the point that they become effectively useless.
>
> I have mixed feelings about breaking a build in these cases (you can't
> deny that it will work to reduce warnings) but I would fully support any
> decision that chose zero-warnings as a goal and that refused to accept new
> code commits if they increased the warning count.
>
> CM
>
> --
> Cliff McCollum <mccol...@amazon.co.uk<mailto:mccol...@amazon.co.uk>>
> Software Development Manager, Core ML, Amazon Cambridge
>
>
>
>
> On 16 Jan 2018, at 16:30, Pedro Larroy <pedro.larroy.li...@gmail.com<
> mailto:pedro.larroy.li...@gmail.com>> wrote:
>
> I understand. What prevents you from disabling the -Werror flag on
> your build configuration?  I don't see where's the big issue. We have
> already tens of flags to configure the build anyway.
>
> I have been fixing every warning that I came across so far in the main
> platforms that I have access. I consider it a practice of leaving
> things cleaner than you find them, and would appreciate if everyone
> else would be a good citizen and do the same. Having to explicitly
    > disable the flag for special cases like the one you mentionwould serve
> this purpose.
>
> Anway, I would be satisfied with at least having warnings as errors on CI.
>
> Regarding my development experience, I just try to make bona fide
> recommendations given that I worked (survived?) in C++ codebases with
> hundreds of developers that run successfull

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread kellen sunderland
@Christopher: I see your point, but the counter argument would be: "Why
should the project run fairly expensive tests (~20 minutes on few GPU
instances) for code that will require you to amend your commit anyway?"  In
normal circumstances I'd completely agree with you and let the full tests
run for information purposes.  However, given how resource intensive our
testing is I'd prefer that we run some sanity checks first before we launch
a full test run.

@Chris + Pedro: I think it's a good idea to focus discussion on what should
happen in CI.  Most CI builds already have warnings as errors turned on (as
mentioned by Xingjian ) because we have USE_DEV=1 set.  We currently have
the following warnings disabled in CI (i.e. they won't fail the build, and
are not reported): '-Wno-unused-variable -Wno-unused-parameter
-Wno-unknown-pragmas -Wno-unused-local-typedefs'.  I think the next step
here would be attempt to cleanup the warnings that we dislike seeing, and
once we have them cleaned up and merged, start failing those warnings on CI.

I agree with Pedro and Cliff that cleaning these up over time would be a
good demonstration of the boy-scout-principle (
https://martinfowler.com/bliki/OpportunisticRefactoring.html).  In my
experience teams and projects that adhere to this principle end up with
much more readable codebases.

Reference build:
http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/master/runs/245/nodes/54/steps/248/log/?start=0

-Kellen

On Tue, Jan 16, 2018 at 5:52 PM, McCollum, Cliff <mccol...@amazon.co.uk>
wrote:

> While you can debate the "broken windows" theory (
> https://en.wikipedia.org/wiki/Broken_windows_theory) I think it has
> relevance to code warnings: the more warnings you tolerate, the more likely
> you are to end up with other undesirable things in your code. My preference
> has always been to treat warnings as errors in any code that leaves a
> developer's own machine. If a team/project doesn't do this, it is relying
> on the good intentions of people to prevent code warnings from multiplying
> to the point that they become effectively useless.
>
> I have mixed feelings about breaking a build in these cases (you can't
> deny that it will work to reduce warnings) but I would fully support any
> decision that chose zero-warnings as a goal and that refused to accept new
> code commits if they increased the warning count.
>
> CM
>
> --
> Cliff McCollum <mccol...@amazon.co.uk<mailto:mccol...@amazon.co.uk>>
> Software Development Manager, Core ML, Amazon Cambridge
>
>
>
>
> On 16 Jan 2018, at 16:30, Pedro Larroy <pedro.larroy.li...@gmail.com<
> mailto:pedro.larroy.li...@gmail.com>> wrote:
>
> I understand. What prevents you from disabling the -Werror flag on
> your build configuration?  I don't see where's the big issue. We have
> already tens of flags to configure the build anyway.
>
> I have been fixing every warning that I came across so far in the main
> platforms that I have access. I consider it a practice of leaving
> things cleaner than you find them, and would appreciate if everyone
> else would be a good citizen and do the same. Having to explicitly
> disable the flag for special cases like the one you mentionwould serve
> this purpose.
>
> Anway, I would be satisfied with at least having warnings as errors on CI.
>
> Regarding my development experience, I just try to make bona fide
> recommendations given that I worked (survived?) in C++ codebases with
> hundreds of developers that run successfully in the order of billions
> of devices. I'm sure you have your own bag of tricks. Please excuse me
> if my comments came across as questioning your development experience
> at any time. I have high regards for your development experience in
> any case.
>
>
>
>
>
>
>
>
>
> On Tue, Jan 16, 2018 at 4:55 PM, Chris Olivier <cjolivie...@gmail.com
> <mailto:cjolivie...@gmail.com>> wrote:
> Pedro,
>
> i don’t know if you’ve ever done much development or not, but during
> development, it’s quite common to comment out arbitrary lines of code,
> create a variable only for debug inspection, or other things that will
> generate warnings, but are actually intentional.  causing a compile error i
> this case would not be acceptable, in my opinion.
>
> as for the any compiler issue, if someone is using a newer gcc or clang,
> and while it only has 2 new warning, they appear in 200 places, are you
> saying it’s the responsibility of this poor community developer to fix all
> of those warnings? or they can open up a JIRA to your team and you will fix
> them?
>
>
> On Tue, Jan 16, 2018 at 7:48 AM Marco de Abreu <
> marco.g.ab...@googlemail.com<mailto:marco.

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread McCollum, Cliff
While you can debate the "broken windows" theory 
(https://en.wikipedia.org/wiki/Broken_windows_theory) I think it has relevance 
to code warnings: the more warnings you tolerate, the more likely you are to 
end up with other undesirable things in your code. My preference has always 
been to treat warnings as errors in any code that leaves a developer's own 
machine. If a team/project doesn't do this, it is relying on the good 
intentions of people to prevent code warnings from multiplying to the point 
that they become effectively useless.

I have mixed feelings about breaking a build in these cases (you can't deny 
that it will work to reduce warnings) but I would fully support any decision 
that chose zero-warnings as a goal and that refused to accept new code commits 
if they increased the warning count.

CM

--
Cliff McCollum <mccol...@amazon.co.uk<mailto:mccol...@amazon.co.uk>>
Software Development Manager, Core ML, Amazon Cambridge




On 16 Jan 2018, at 16:30, Pedro Larroy 
<pedro.larroy.li...@gmail.com<mailto:pedro.larroy.li...@gmail.com>> wrote:

I understand. What prevents you from disabling the -Werror flag on
your build configuration?  I don't see where's the big issue. We have
already tens of flags to configure the build anyway.

I have been fixing every warning that I came across so far in the main
platforms that I have access. I consider it a practice of leaving
things cleaner than you find them, and would appreciate if everyone
else would be a good citizen and do the same. Having to explicitly
disable the flag for special cases like the one you mentionwould serve
this purpose.

Anway, I would be satisfied with at least having warnings as errors on CI.

Regarding my development experience, I just try to make bona fide
recommendations given that I worked (survived?) in C++ codebases with
hundreds of developers that run successfully in the order of billions
of devices. I'm sure you have your own bag of tricks. Please excuse me
if my comments came across as questioning your development experience
at any time. I have high regards for your development experience in
any case.









On Tue, Jan 16, 2018 at 4:55 PM, Chris Olivier 
<cjolivie...@gmail.com<mailto:cjolivie...@gmail.com>> wrote:
Pedro,

i don’t know if you’ve ever done much development or not, but during
development, it’s quite common to comment out arbitrary lines of code,
create a variable only for debug inspection, or other things that will
generate warnings, but are actually intentional.  causing a compile error i
this case would not be acceptable, in my opinion.

as for the any compiler issue, if someone is using a newer gcc or clang,
and while it only has 2 new warning, they appear in 200 places, are you
saying it’s the responsibility of this poor community developer to fix all
of those warnings? or they can open up a JIRA to your team and you will fix
them?


On Tue, Jan 16, 2018 at 7:48 AM Marco de Abreu 
<marco.g.ab...@googlemail.com<mailto:marco.g.ab...@googlemail.com>>
wrote:

So you're proposing to have a stage AFTER test execution which would report
warnings as errors? While this is a good idea, I'm afraid that a fail-fast
would also have its benefits - especially considering that compilation only
takes a few minutes and consumes few resources while test execution takes
up most of the time and is very costly.

-Marco

On Tue, Jan 16, 2018 at 4:11 PM, Barber, Christopher <
christopher.bar...@analog.com<mailto:christopher.bar...@analog.com>> wrote:

Personally, I don’t like treating warnings as errors because it prevents
compilation from completing and causes you to lose any ability to test
the
code and get any other information. Killing the build because of a failed
warning for something that might not matter means that you may not find
out
about other important test failures until much later. Better to add a
test
that grovels the build logs for warning messages and treat it as a test
failure.

I also prefer to only enable exactly those warnings that truly matter.

On 1/16/18, 8:23 AM, "Marco de Abreu" 
<marco.g.ab...@googlemail.com<mailto:marco.g.ab...@googlemail.com>>
wrote:

   I'd vote for having warnings as errors only for CI but not in general
   builds which are getting executed by users on their local machine.
Just in
   case CI misses a warning due to a different version, this could
block a
   developer from compiling MXNet locally even though it might just be a
   warning which is not critical enough (otherwise it would be an error)
to
   justify blocking the compilation. In my opinion, it would be good if
we can
   filter most warnings during PR-stage and risk that some are getting
into
   the master branch due to a different compiler version. A reduction of
(for
   example) 95% without risking to break the master branch on different
   compilers is way better in my opinion than having a 100% coverage
which
   could 

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Pedro Larroy
I understand. What prevents you from disabling the -Werror flag on
your build configuration?  I don't see where's the big issue. We have
already tens of flags to configure the build anyway.

I have been fixing every warning that I came across so far in the main
platforms that I have access. I consider it a practice of leaving
things cleaner than you find them, and would appreciate if everyone
else would be a good citizen and do the same. Having to explicitly
disable the flag for special cases like the one you mentionwould serve
this purpose.

Anway, I would be satisfied with at least having warnings as errors on CI.

Regarding my development experience, I just try to make bona fide
recommendations given that I worked (survived?) in C++ codebases with
hundreds of developers that run successfully in the order of billions
of devices. I'm sure you have your own bag of tricks. Please excuse me
if my comments came across as questioning your development experience
at any time. I have high regards for your development experience in
any case.









On Tue, Jan 16, 2018 at 4:55 PM, Chris Olivier <cjolivie...@gmail.com> wrote:
> Pedro,
>
> i don’t know if you’ve ever done much development or not, but during
> development, it’s quite common to comment out arbitrary lines of code,
> create a variable only for debug inspection, or other things that will
> generate warnings, but are actually intentional.  causing a compile error i
> this case would not be acceptable, in my opinion.
>
> as for the any compiler issue, if someone is using a newer gcc or clang,
> and while it only has 2 new warning, they appear in 200 places, are you
> saying it’s the responsibility of this poor community developer to fix all
> of those warnings? or they can open up a JIRA to your team and you will fix
> them?
>
>
> On Tue, Jan 16, 2018 at 7:48 AM Marco de Abreu <marco.g.ab...@googlemail.com>
> wrote:
>
>> So you're proposing to have a stage AFTER test execution which would report
>> warnings as errors? While this is a good idea, I'm afraid that a fail-fast
>> would also have its benefits - especially considering that compilation only
>> takes a few minutes and consumes few resources while test execution takes
>> up most of the time and is very costly.
>>
>> -Marco
>>
>> On Tue, Jan 16, 2018 at 4:11 PM, Barber, Christopher <
>> christopher.bar...@analog.com> wrote:
>>
>> > Personally, I don’t like treating warnings as errors because it prevents
>> > compilation from completing and causes you to lose any ability to test
>> the
>> > code and get any other information. Killing the build because of a failed
>> > warning for something that might not matter means that you may not find
>> out
>> > about other important test failures until much later. Better to add a
>> test
>> > that grovels the build logs for warning messages and treat it as a test
>> > failure.
>> >
>> > I also prefer to only enable exactly those warnings that truly matter.
>> >
>> > On 1/16/18, 8:23 AM, "Marco de Abreu" <marco.g.ab...@googlemail.com>
>> > wrote:
>> >
>> > I'd vote for having warnings as errors only for CI but not in general
>> > builds which are getting executed by users on their local machine.
>> > Just in
>> > case CI misses a warning due to a different version, this could
>> block a
>> > developer from compiling MXNet locally even though it might just be a
>> > warning which is not critical enough (otherwise it would be an error)
>> > to
>> > justify blocking the compilation. In my opinion, it would be good if
>> > we can
>> > filter most warnings during PR-stage and risk that some are getting
>> > into
>> > the master branch due to a different compiler version. A reduction of
>> > (for
>> > example) 95% without risking to break the master branch on different
>> > compilers is way better in my opinion than having a 100% coverage
>> which
>> > could block compilation - especially because we would only notice if
>> a
>> > user
>> > tells us afterwards.
>> >
>> > -Marco
>> >
>> > On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <
>> > pedro.larroy.li...@gmail.com>
>> > wrote:
>> >
>> > > 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 bug

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Chris Olivier
Pedro,

i don’t know if you’ve ever done much development or not, but during
development, it’s quite common to comment out arbitrary lines of code,
create a variable only for debug inspection, or other things that will
generate warnings, but are actually intentional.  causing a compile error i
this case would not be acceptable, in my opinion.

as for the any compiler issue, if someone is using a newer gcc or clang,
and while it only has 2 new warning, they appear in 200 places, are you
saying it’s the responsibility of this poor community developer to fix all
of those warnings? or they can open up a JIRA to your team and you will fix
them?


On Tue, Jan 16, 2018 at 7:48 AM Marco de Abreu <marco.g.ab...@googlemail.com>
wrote:

> So you're proposing to have a stage AFTER test execution which would report
> warnings as errors? While this is a good idea, I'm afraid that a fail-fast
> would also have its benefits - especially considering that compilation only
> takes a few minutes and consumes few resources while test execution takes
> up most of the time and is very costly.
>
> -Marco
>
> On Tue, Jan 16, 2018 at 4:11 PM, Barber, Christopher <
> christopher.bar...@analog.com> wrote:
>
> > Personally, I don’t like treating warnings as errors because it prevents
> > compilation from completing and causes you to lose any ability to test
> the
> > code and get any other information. Killing the build because of a failed
> > warning for something that might not matter means that you may not find
> out
> > about other important test failures until much later. Better to add a
> test
> > that grovels the build logs for warning messages and treat it as a test
> > failure.
> >
> > I also prefer to only enable exactly those warnings that truly matter.
> >
> > On 1/16/18, 8:23 AM, "Marco de Abreu" <marco.g.ab...@googlemail.com>
> > wrote:
> >
> > I'd vote for having warnings as errors only for CI but not in general
> > builds which are getting executed by users on their local machine.
> > Just in
> > case CI misses a warning due to a different version, this could
> block a
> > developer from compiling MXNet locally even though it might just be a
> > warning which is not critical enough (otherwise it would be an error)
> > to
> > justify blocking the compilation. In my opinion, it would be good if
> > we can
> > filter most warnings during PR-stage and risk that some are getting
> > into
> > the master branch due to a different compiler version. A reduction of
> > (for
> > example) 95% without risking to break the master branch on different
> > compilers is way better in my opinion than having a 100% coverage
> which
> > could block compilation - especially because we would only notice if
> a
> > user
> > tells us afterwards.
> >
> > -Marco
> >
> > On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <
> > pedro.larroy.li...@gmail.com>
> > wrote:
> >
> > > 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
> > > n

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Marco de Abreu
So you're proposing to have a stage AFTER test execution which would report
warnings as errors? While this is a good idea, I'm afraid that a fail-fast
would also have its benefits - especially considering that compilation only
takes a few minutes and consumes few resources while test execution takes
up most of the time and is very costly.

-Marco

On Tue, Jan 16, 2018 at 4:11 PM, Barber, Christopher <
christopher.bar...@analog.com> wrote:

> Personally, I don’t like treating warnings as errors because it prevents
> compilation from completing and causes you to lose any ability to test the
> code and get any other information. Killing the build because of a failed
> warning for something that might not matter means that you may not find out
> about other important test failures until much later. Better to add a test
> that grovels the build logs for warning messages and treat it as a test
> failure.
>
> I also prefer to only enable exactly those warnings that truly matter.
>
> On 1/16/18, 8:23 AM, "Marco de Abreu" <marco.g.ab...@googlemail.com>
> wrote:
>
> I'd vote for having warnings as errors only for CI but not in general
> builds which are getting executed by users on their local machine.
> Just in
> case CI misses a warning due to a different version, this could block a
> developer from compiling MXNet locally even though it might just be a
> warning which is not critical enough (otherwise it would be an error)
> to
> justify blocking the compilation. In my opinion, it would be good if
> we can
> filter most warnings during PR-stage and risk that some are getting
> into
> the master branch due to a different compiler version. A reduction of
> (for
> example) 95% without risking to break the master branch on different
> compilers is way better in my opinion than having a 100% coverage which
> could block compilation - especially because we would only notice if a
> user
> tells us afterwards.
>
> -Marco
>
> On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <
> pedro.larroy.li...@gmail.com>
> wrote:
>
> > 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 <
> cjolivie...@gmail.com>
> > 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
> 

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Barber, Christopher
Personally, I don’t like treating warnings as errors because it prevents 
compilation from completing and causes you to lose any ability to test the code 
and get any other information. Killing the build because of a failed warning 
for something that might not matter means that you may not find out about other 
important test failures until much later. Better to add a test that grovels the 
build logs for warning messages and treat it as a test failure.

I also prefer to only enable exactly those warnings that truly matter.

On 1/16/18, 8:23 AM, "Marco de Abreu" <marco.g.ab...@googlemail.com> wrote:

I'd vote for having warnings as errors only for CI but not in general
builds which are getting executed by users on their local machine. Just in
case CI misses a warning due to a different version, this could block a
developer from compiling MXNet locally even though it might just be a
warning which is not critical enough (otherwise it would be an error) to
justify blocking the compilation. In my opinion, it would be good if we can
filter most warnings during PR-stage and risk that some are getting into
the master branch due to a different compiler version. A reduction of (for
example) 95% without risking to break the master branch on different
compilers is way better in my opinion than having a 100% coverage which
could block compilation - especially because we would only notice if a user
tells us afterwards.

-Marco

On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <pedro.larroy.li...@gmail.com>
wrote:

> 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 <cjolivie...@gmail.com>
> 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 <
> > marco.g.ab...@googlemail.com> wrote:
> >
    > >> +1
> >>
> >> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> >> pedro.larroy.li...@gmail.com>
> >> wrote:
> >>
> >> > Hi
> >> >
> >> > I would like to propose to compile in CI with warnings as errors for
> >> > increased code quality. This has a dual 

Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Marco de Abreu
I'd vote for having warnings as errors only for CI but not in general
builds which are getting executed by users on their local machine. Just in
case CI misses a warning due to a different version, this could block a
developer from compiling MXNet locally even though it might just be a
warning which is not critical enough (otherwise it would be an error) to
justify blocking the compilation. In my opinion, it would be good if we can
filter most warnings during PR-stage and risk that some are getting into
the master branch due to a different compiler version. A reduction of (for
example) 95% without risking to break the master branch on different
compilers is way better in my opinion than having a 100% coverage which
could block compilation - especially because we would only notice if a user
tells us afterwards.

-Marco

On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <pedro.larroy.li...@gmail.com>
wrote:

> 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 <cjolivie...@gmail.com>
> 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 <
> > marco.g.ab...@googlemail.com> wrote:
> >
> >> +1
> >>
> >> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> >> pedro.larroy.li...@gmail.com>
> >> 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.
> >> >
> >>
>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-16 Thread Pedro Larroy
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 <cjolivie...@gmail.com> 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 <
> marco.g.ab...@googlemail.com> wrote:
>
>> +1
>>
>> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
>> pedro.larroy.li...@gmail.com>
>> 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.
>> >
>>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Xingjian SHI
It seems we have the `DEV` mode 
(https://github.com/apache/incubator-mxnet/blob/master/make/config.mk#L28) to 
do that? Is it correct?

Xingjian


From: Ziyue Huang <zyhuan...@gmail.com>
Sent: Tuesday, January 16, 2018 9:01 AM
To: dev@mxnet.incubator.apache.org
Subject: Re: Proposal for treating warnings as errors in Linux & Clang builds 
(-Werror)

+1 since warnings might be potential errors. In my perspective, an
excellent project is better to have 0 warnings.

Chris Olivier <cjolivie...@gmail.com>于2018年1月16日 周二上午2:27写道:

> 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 <
> marco.g.ab...@googlemail.com> wrote:
>
> > +1
> >
> > On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> > pedro.larroy.li...@gmail.com>
> > 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
[https://avatars0.githubusercontent.com/u/928489?s=400=4]<https://github.com/apache/incubator-mxnet/pull/9398>

[Make] add -Werror, warnings as errors by larroy ・ Pull Request #9398 ・ 
apache/incubator-mxnet<https://github.com/apache/incubator-mxnet/pull/9398>
github.com
incubator-mxnet - Lightweight, Portable, Flexible Distributed/Mobile Deep 
Learning with Dynamic, Mutation-aware Dataflow Dep Scheduler; for Python, R, 
Julia, Scala, Go, Javascript and more



> > >
> > > http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/
> > > incubator-mxnet/detail/PR-9398/1/pipeline
> > >
> > > Pedro.
> > >
> >
>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Yuan Tang
+1

On Mon, Jan 15, 2018 at 8:02 PM Ziyue Huang <zyhuan...@gmail.com> wrote:

> +1 since warnings might be potential errors. In my perspective, an
> excellent project is better to have 0 warnings.
>
> Chris Olivier <cjolivie...@gmail.com>于2018年1月16日 周二上午2:27写道:
>
> > 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 <
> > marco.g.ab...@googlemail.com> wrote:
> >
> > > +1
> > >
> > > On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> > > pedro.larroy.li...@gmail.com>
> > > 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.
> > > >
> > >
> >
>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Ziyue Huang
+1 since warnings might be potential errors. In my perspective, an
excellent project is better to have 0 warnings.

Chris Olivier <cjolivie...@gmail.com>于2018年1月16日 周二上午2:27写道:

> 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 <
> marco.g.ab...@googlemail.com> wrote:
>
> > +1
> >
> > On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> > pedro.larroy.li...@gmail.com>
> > 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.
> > >
> >
>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Chris Olivier
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 <
marco.g.ab...@googlemail.com> wrote:

> +1
>
> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> pedro.larroy.li...@gmail.com>
> 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.
> >
>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Haibin Lin
+1 (binding)

On Mon, Jan 15, 2018 at 9:43 AM, Marco de Abreu <
marco.g.ab...@googlemail.com> wrote:

> +1
>
> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
> pedro.larroy.li...@gmail.com>
> 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.
> >
>


Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Marco de Abreu
+1

On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <pedro.larroy.li...@gmail.com>
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.
>


Proposal for treating warnings as errors in Linux & Clang builds (-Werror)

2018-01-15 Thread Pedro Larroy
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.