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