[PATCH] D123654: [NFC][X86][CodeGen] Use ArrayRef in TargetLowering functions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D123654#3476841 <https://reviews.llvm.org/D123654#3476841>, @RKSimon wrote:

> This just makes the code much harder to decipher
>
> Are you aware of any performance improvement from this change? The code is called so little I'm not expecting much benefit TBH

I tend to agree as well. The flexibility of the API can be useful, but blanket changing to use the API as much as possible don't seem like a good idea. In the old style you could do a quick scan up and down to see which opcodes which opcodes and types were affected because we oftened lined things up in columns and ignored clang-format when it wanted to change it. With this change you have to scan across an entire line and sometimes across multiple lines to understand it and it makes visually comparing how different operations are handled harder.

I don't object as much to grouping things like the signed and unsigned form of an operation together. Or very related operations like add and sub. But changes that group a set of unrelated operations together just to make the file shorter I think goes too far.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123654/new/

https://reviews.llvm.org/D123654