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

Ahmed Bougacha ahmed.bougacha at gmail.com
Wed Aug 21 02:04:18 PDT 2013


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.

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.

> 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
>
>



More information about the llvm-commits mailing list