Could you please explain the purpose of this patch, since mintty (I use it myself though as as the frontend for wslbridge) is not a native Windows console in any shape or form, unless I'm missing something? In either case this shouldn't really be necessary unless your setup involves running LLVM tools on Windows through mintty through Windows console.
As far as I understand, mintty is a xterm-256color compatible terminal emulator, so I'm not entirely sure why you're using Windows console APIs either?
If I may ask, could you write up a detailed explanation of what this patch is aiming to achieve and what specific problem does it solve?
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).
If we do decide to do this, I don't think it's the kind of thing we can reasonably test in LLVM, for the reasons @kristina mentioned. If people agree it's a valuable feature that's worth the complexity, I think we can commit it, and rely on mintty users to tell us when the colors don't end up printing.
This is plain old win32 API code. Why make it mingw-specific? Then clang-cl will use colors in mintty. I previously used it as my primary development environment, even for targeting MSVC. Today I use cmd, since it's improved greatly, but it's still pretty janky.
I think if there's interest in this and mock-based tests are too difficult, it's possible to have one of the Windows builders run a very specific test with mintty just so we can watch out if something does go wrong. That said, I think for now it should need an opt-in (possibly through -mllvm) flag, and if we do get one or more Windows builders to run it (it should be relatively lightweight as it's literally just testing the support library) and it remains stable across mintty releases, and someone maintains it (I'm up for that, can even supply a builder just for this specific, hypothetical test), the opt-in requirement could eventually be dropped in favor of an "auto" style thing just as with other color diagnostics.
I may be overthinking it, but I think that's a semi-reasonable plan for rolling this out and I think a tool-style test with FileCheck would be the most appropriate here?
Anyway, would like to hear some opinions and seeing how many are interested in such a feature and what they think of the roll-out plan I proposed, despite it sounding slightly (over-)complicated.
I don't think we want to try to maintain some kind of mintty integration test harness infrastructure. It's hard to make these kinds of tests reliable, and when they fail, it's often a signal that something in the environment changed, not LLVM, like a new version of mintty that uses differently named pipes.
And, even if we had this test and it worked, it's not the kind of information an LLVM contributor wants to get from a buildbot somewhere, or a new contributor the first time they run the test suite on a new machine. We want the test suite to have a high likelihood of passing by default, with as few environmental requirements of dependencies as possible. Our coreutils dependence here gets in the way on Windows, of course, and causes all kinds of problems.
If we were to test this, I'd suggest using a unit test that creates appropriately named pipes and re-lauches itself as a subprocess and then emits some colored output to stdout. The parent process would read from the named pipe and look for some ANSI escape codes.
I think the XP ship has sailed. I think our STL requirements now require using an MSVC CRT version that doesn't support XP.
I would also like this feature in MSVC builds, and less ifdefs are good if possible. I'm willing to apply this patch locally and fix any compatibility issues in it.