AARCH64_BE load/store rules fix to conform to ARM ABI

Albrecht Kadlec akadlec at a-bix.com
Tue Feb 25 08:59:16 PST 2014


Hi Renato,

there're a few misunderstandings:

First, I'm not Christian (and he had a reviewer explicitly asking for 
those _related_ indentations, which brings up the question about global 
policy.)

The -w patch was supplied IN_ADDITION and only for faster understanding 
by those who wrote these AArch64*.td files in the first place.
Otherwise it's looking as if I changed 30% of the matcher - which isn't 
even close to reality.

No clang-format has harmed any unrelated white-spaces.
Any indentations were done by hand to reflect the structure of the 
_resulting_ AArch64*.td rule code.

If I try to preserve the indentation, using Requires<[IsLE]> per rule 
instead of the braces, all the lines that are now whitespace-diff-only 
will become real differences.

I'm fine with either, but will wait for the AArch64*.td reviewers' 
advice/LGTM - no one used this construct for AArch64 so far.

I'm not going to spend time discussing whether mis-indented 
spaghetti-code is better or worse than an incorrect "svn blame" - 
neither is: they're both ugly as hell.

BTW: did you know: "svn blame" also seems to accept "-x -w" to do the 
right thing for whitespaces.

cheers,
Albrecht


On 2014-02-25 17:14, Renato Golin wrote:
> 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