On Mon, Aug 22, 2016 at 10:34 AM, Ben Craig <ben.cr...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
