[PATCH] D23279: clang-reorder-fields

[Aaron Ballman via cfe-commits]

On Mon, Aug 22, 2016 at 10:34 AM, Ben Craig <ben.craig at codeaurora.org> wrote:
> bcraig added a comment.
>
> I will note that I'm only really looking at the tests, mostly from the perspective of a potential user.
>
> Thanks for getting C++ initializer lists in.
>
> The tests in general look fine.  The omission of cross visibility reordering is fine as an initial step.
>
> Once multi-TU support is in, you'll definitely want a test for that.
>
>
> ================
> Comment at: test/clang-reorder-fields/ClassMixedInitialization.cpp:1-3
> @@ +1,4 @@
> +// RUN: cat %s > %t.cpp
> +// RUN: clang-reorder-fields -record-name Foo -fields-order e,x,pi,v,s %t.cpp -i -- -std=c++11
> +// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
> +
> ----------------
> Nit:
> This general pattern seems suspicious to me.  It really seems like it should be some variation of "clang-reorder-fields [...] | FileCheck %s", without the cat and sed stuff.  Is there a clang-reorder-fields mode where changes are output into a new file / stdout instead of modifying the file in place?

This pattern likely also means the tests will fail on Windows (every
instance of RUN: sed in the clang tests also has REQUIRES: shell,
which disables the tests on Windows). Removing a test reliance on sed
is pretty important, IMO.

~Aaron