[PATCH] D45138: [MC] Change std::sort to llvm::sort in response to r327219

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 12 13:59:51 PDT 2018


They are not really that large. 2 out of 4 are just a few lines (or even a
single line) change. I wouldn't worry about them to be broken. But that's
not really important. I'll go ahead and approve.

On Thu, Apr 12, 2018 at 1:57 PM Grang, Mandeep Singh <mgrang at codeaurora.org>
wrote:

> Actually, I would prefer them to be separate. Makes it easier to track
> builtbot breakage and revert if needed. Also since all the other llvm
> patches to replace std::sort to llvm::sort have been small target/area
> specific I would like to follow the same for these four.
>
> Thanks,
> Mandeep
>
> On 4/12/2018 1:52 PM, Rui Ueyama wrote:
>
> Could you just merge these four changes in a single patch?
>
> On Thu, Apr 12, 2018 at 1:51 PM Rui Ueyama <ruiu at google.com> wrote:
>
>> Since these are mechanical changes, I'll review and give LGTM.
>>
>> On Thu, Apr 12, 2018 at 1:49 PM Grang, Mandeep Singh <
>> mgrang at codeaurora.org> wrote:
>>
>>> @ruiu @rksimon These 4 are the last remaining patches pending review and
>>> merge. Is it possible for you to review them?
>>> https://reviews.llvm.org/D45142
>>> https://reviews.llvm.org/D45139
>>> https://reviews.llvm.org/D45138
>>> https://reviews.llvm.org/D45137
>>>
>>> --Mandeep
>>>
>>> On 4/9/2018 3:34 PM, Grang, Mandeep Singh wrote:
>>>
>>>
>>> On 4/9/2018 3:16 PM, Rui Ueyama wrote:
>>>
>>> On Mon, Apr 9, 2018 at 3:13 PM Mandeep Singh Grang via Phabricator <
>>> reviews at reviews.llvm.org> wrote:
>>>
>>>> mgrang added a comment.
>>>>
>>>> In https://reviews.llvm.org/D45138#1062213, @ruiu wrote:
>>>>
>>>> > I'm not familiar with r327219, but if a decision to use randomized
>>>> sort has been made, why don't you replace all occurrences of std::sort with
>>>> llvm::sort in a single patch? I don't think that an author of each file
>>>> don't have to understand this kind of change and approve individually.
>>>>
>>>>
>>>> I had a patch to replace *all* occurrences of std::sort to llvm::sort
>>>> https://reviews.llvm.org/D44363. However, the reviewers felt that the
>>>> patch was getting too big and suggested splitting it into smaller patches
>>>> target-wise, tablegen, MC, etc. Hence these separate patches.
>>>>
>>>
>>> If the only reason not to submit them as a single patch is its size, I
>>> can actually approve. As long as it is a mechanical patch, I'm not worried
>>> too much about its size.
>>>
>>> Adding @rksimon since he has been reviewing my patches. Is it OK if I
>>> squash the remaining 4 patches (D45142 D45139 D45138 D45137) into one
>>> and push them as a single patch?
>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180412/ac83a3da/attachment-0001.html>


More information about the llvm-commits mailing list