[PATCH] D84233: [lit] Escape ANSI control character in xunit output

Mon Jul 27 10:27:59 PDT 2020

jdenny accepted this revision.
jdenny added a comment.
This revision is now accepted and ready to land.

Other than the nit I just added about comment duplication, this LGTM.  Thanks.  However, let's a wait a couple of days in case anyone else has an opinion.

I still wonder whether it would be more helpful to drop control characters rather than irreversibly escape them.  Dropping them makes the output more readable.  Reversibly escaping them makes it possible to feed the text to a tool that can render the control codes.  Irreversibly escaping them doesn't achieve either of those advantages, and I'm not sure if it serves a real use case.  Any comments on this question?

================
Comment at: llvm/utils/lit/lit/reports.py:142
+            # JUnit XML will throw an exception when ecountering those
+            # characters and similar problems also occur with GitLab CI.
+            output = escape_invalid_xml_chars(output)
----------------
You indeed did what I asked, but I didn't consider that we would end up with duplication of some comments at the caller and callee.

I think it would be better to consolidate these comments with the ones you just inserted into the `escape_invalid_xml_chars` definition, and place the consolidated version as header comments on `escape_invalid_xml_chars` and nowhere else.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84233/new/

https://reviews.llvm.org/D84233