AARCH64_BE load/store rules fix to conform to ARM ABI

Renato Golin renato.golin at linaro.org
Tue Feb 25 08:14:38 PST 2014


On 25 February 2014 14:30, Albrecht Kadlec <akadlec at a-bix.com> wrote:
> The commit will have correct indentation.

Please, don't do this.

Your other patch was created ignoring white spaces and we reviewed
based on that, but the patch that you actually submitted had "fixed
indentation".

While Clang auto-format is a nice tool, changing the indentation of
existing code destroys the change history for no purpose. Worse, it
meddles with parts of the file that didn't need changing, making it
harder to apply subsequent patches (we'll all have to rebase our
patches and fix the conflicts), harder to revert (in case anything
goes wrong with one of the bots or private tests), and harder to
review (even with -w, some irrelevant changes do creep up).

All independent formatting changes should go as a separate commit, and
even so, I can't see real value in that. If we all start changing the
formatting to whatever we like, or Clang likes, we'll end up with
nothing but formatting changes to review. The Clang formatter changes
with time, and the preference of each one of us is probably a
completely disjoint set when taken all preferences into account, so,
please, just don't.

The only changes that are acceptable are things that enhance clarity
(like early exits), or remove violations of the coding standard (like
80-column, etc). The former is good, the latter... I still have my
reserves. Of course, if you're changing a line for a good cause, feel
free to format it according to the standards, correct any violation,
whitespace, etc.

So, I'd recommend you to revert your clang-format, apply the clean
patch, make sure it doesn't break anything (check-all, etc) and submit
the *real* patch for consideration.

cheers,
--renato

http://llvm.org/docs/CodingStandards.html
http://llvm.org/docs/DeveloperPolicy.html



More information about the llvm-commits mailing list