[llvm] r188872 - MC CFG: uint64_t -> size_t for vector size.

[Chandler Carruth]

On Wed, Aug 21, 2013 at 2:04 AM, Ahmed Bougacha <ahmed.bougacha at gmail.com>wrote:

> On Wed, Aug 21, 2013 at 1:18 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
> > Ahmed, first please don't build up such a large bank of commits and push
> > them all at once.
> >
> > This first causes significant load on the reviewers that is hard to
> predict
> > and absorb. Secondly, it makes build bots useless at tell you which
> commit
> > broke something and allowing fine grained rollbacks.
> >
> > Please mail and commit a patch as soon as it is ready / approved.
> >
> >
> > Second, while this patch is clearly trivial, and I saw that the first
> last
> > of this long chain of patches introduced some testing facilities,
> *please*
> > introduce the test harness *first*, and then have incremental tests
> attached
> > to each commit so reviewers can see what has been fixed.
>
> I actually had all this in mind when committing, sorry for that.
> The thing is that most of these are (varyingly) trivial, building for
> 2 changes: one has its test (r188879), the other enables upcoming
> tests. Most changes really are either drive-by cleanups or adding some
> useful method here and there. As to why not gradually, the tiny
> commits originated from splitting the 2 major ones, but that's another
> problem.
>

While generally, this seems like a good strategy, your summary doesn't
really match my skimming over the commits. They seemed about half and half
"fix something up" (ie, needs a test case), and "add this functionality for
a subsequent patch".

The latter are actually really nice, but really should go in after the
design / direction of the final patch has gone through review.

It absolutely could be that I've mis-skimmed the commits. Sorry for that.
However, 20 at a time is too many to easily do anything more than skim.
I'll re-read them as I get time.

The real test users are still coming, I tried to push this now exactly
> not to have an overwhelming set of changes at once; clearly I overshot
> even with this.
>

Yea, I think you'll need to work to get a more incremental process.

The one I recommend for folks hacking somewhere that they need some
pre-commit review just for design / direction is to work very hard to split
things out early, and the moment they are split out mail them out for
review and start that process. The key is to overlap that process with the
process of testing the core change, and getting that (usually larger)
change reviewed so that there is a more continuous stream rather than
bursts.


>
> > Finally, it doesn't look like any of this series really got reviewed
> before
> > committing. The last review I see with anything to do with MC CFG
> > construction was back in May, and you seemed to think pre-commit review
> was
> > appropriate there, so I don't know what some of these (not this one, I
> just
> > really don't want to write you N different emails with feedback) didn't
> need
> > review first.
>
> Thought about relying on post-commit review for this; the last was
> actually the introduction of the MC CFG facilities, mostly to make
> sure the general idea was right.
>
> Again sorry about that!
>
> -- Ahmed
>
> > -Chandler
> >
> >
> >
> > On Wed, Aug 21, 2013 at 12:27 AM, Ahmed Bougacha <
> ahmed.bougacha at gmail.com>
> > wrote:
> >>
> >> Author: ab
> >> Date: Wed Aug 21 02:27:44 2013
> >> New Revision: 188872
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=188872&view=rev
> >> Log:
> >> MC CFG: uint64_t -> size_t for vector size.
> >>
> >> Modified:
> >>     llvm/trunk/include/llvm/MC/MCAtom.h
> >>
> >> Modified: llvm/trunk/include/llvm/MC/MCAtom.h
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCAtom.h?rev=188872&r1=188871&r2=188872&view=diff
> >>
> >>
> ==============================================================================
> >> --- llvm/trunk/include/llvm/MC/MCAtom.h (original)
> >> +++ llvm/trunk/include/llvm/MC/MCAtom.h Wed Aug 21 02:27:44 2013
> >> @@ -139,7 +139,7 @@ public:
> >>
> >>    const MCDecodedInst &back() const { return Insts.back(); }
> >>    const MCDecodedInst &at(size_t n) const { return Insts.at(n); }
> >> -  uint64_t size() const { return Insts.size(); }
> >> +  size_t size() const { return Insts.size(); }
> >>    /// @}
> >>
> >>    /// \name Atom type specific split/truncate logic.
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130821/fa1a39b1/attachment.html>