[PATCH] D53567: [Support] Enable color diagnostics for mintty

[Kristina Brooks via Phabricator via llvm-commits]

kristina requested changes to this revision.
kristina added a reviewer: rnk.
kristina added a subscriber: rnk.
kristina added a comment.
This revision now requires changes to proceed.

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).


Repository:
  rL LLVM

https://reviews.llvm.org/D53567