mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Carin Meier <carinme...@gmail.com>
Subject Re: Should PR-860 (Use modernized range loops where possible) be reverted?
Date Tue, 20 Nov 2018 16:34:05 GMT
Great. Thanks for the clarifications everyone. I think we are good then :)

- Carin

On Tue, Nov 20, 2018 at 10:43 AM kellen sunderland <
kellen.sunderland@gmail.com> wrote:

> Hey Carin, I don't think there's any issues merging this PR.  The veto'd
> aspect was around _requiring_ modern loop usage, and failing the build if
> clang tidy detected modern loops could be used but weren't.  The original
> PR included a check for this and would fail any builds not using modern
> loops.  Several people didn't like this aspect so I updated the PR and
> removed that overly-strict check.  The current PR doesn't have anything it
> in that's been vetod.  We're continuing to warn if clang-tidy detects a
> loop could be modernized, but are not causing an error (which was actually
> the behaviour before this PR was merged).
>
> On Tue, Nov 20, 2018 at 7:29 AM Anton Chernov <mechernov@gmail.com> wrote:
>
> > Hi Carin,
> >
> > The discussion [1] was about whether to enable automatic checks on using
> > old behaviour in new PR's. Kellens PR [2] was about modernizing the
> actual
> > code itself and was not up for voting, thus could not receive any
> technical
> > veto votes.
> >
> > Per the discussion (as I have understood it), we won't get veto votes if
> we
> > would enable the check on CI, if it would be treated as a warning.
> >
> > Thank you for merging the PR in the first place. I see no reason for
> > reverting it.
> >
> > Best
> > Anton
> >
> > [1]
> >
> >
> https://lists.apache.org/thread.html/b47f285a80bef47c5ead6c361614e338a0661f6c0c76196c1e3719c5@%3Cdev.mxnet.apache.org%3E
> > [2] https://github.com/apache/incubator-mxnet/pull/12356
> >
> >
> > вт, 20 нояб. 2018 г. в 15:24, Pedro Larroy <pedro.larroy.lists@gmail.com
> >:
> >
> > > Hi all
> > >
> > > I think we have to make the clear separation between the thread votes
> > > on "uniformly adopting C++11 range loops in the MXNet project" and a
> > > PR which refactored code to be more legible and with improved variable
> > > names.
> > > Merging that PR doesn't imply that we have to uniformly adopt the
> > > previous proposal.  The PR was reviewed and approved by several
> > > people. I would keep the two topics separate, merging this PR doesn't
> > > prescribe any particular idiom for future commits or reviews.
> > >
> > > Pedro.
> > >
> > > On Tue, Nov 20, 2018 at 2:58 PM Carin Meier <carinmeier@gmail.com>
> > wrote:
> > > >
> > > > My intent was to be helpful, but I think I may have merged this PR
> > > > yesterday too soon thinking it was approved and ready to merge
> > > > https://github.com/apache/incubator-mxnet/pull/12356
> > > >
> > > > I didn't see the connected dev discussion
> > > >
> > >
> >
> https://lists.apache.org/thread.html/b47f285a80bef47c5ead6c361614e338a0661f6c0c76196c1e3719c5@%3Cdev.mxnet.apache.org%3E
> > > > where there were -1 votes, which I believe are vetos?
> > > >
> > > > So the question is confirm: should PR should be reverted?
> > > >
> > > > Sorry for any confusion,
> > > > Carin
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message