boomanaiden154 wrote: > The previous, uglified lld tests, and this, make me believe that we should > revisit the lit feature. I am not sure I agree with this test rewriting.
I understand the argument that the tests in #156526 look a bit worse. It's not an opinion that everyone shares though. @petrhosek thinks it makes the tests more readable because you no longer need to know about subshells to understand the semantics of the test. For this test though, modifying it to use python adds a bit of boilerplate, but I think significantly clarifies what the test is actually trying to test. Instead of using arguably a hack with subshells to create a fd with a non-zero offset for `llvm-mc` to output into, we can explicitly create exactly that in python with easily readable code. Overall, there are three options I can see for this test. 1. Rewrite in python (what this patch currently does). 2. Rewrite to invoke the subshell through `bash` first. 3. Implement subshells in lit. I think 1 makes the best set of tradeoffs. I'm not opposed to 3, but I would like a reasonably strong consensus that the handful of tests impacted by this are objectively worse in terms of readability/cleanliness before doing that. Everyone with knowledge of lit's internal shell that I have talked about subshells thinks it would be a poor decision to implement them. I have not personally done enough digging/attempted enough of an implementation to know the exact tradeoffs though. https://github.com/llvm/llvm-project/pull/157232 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
