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

Grang, Mandeep Singh via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 12 13:57:22 PDT 2018


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 
> <mailto: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 <mailto: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
>>>         <mailto: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
>>         (D45142D45139D45138D45137) 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/33511a9a/attachment.html>


More information about the llvm-commits mailing list