[PATCH] D97472: [test] Use host platform specific error message substitution in lit tests

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

In D97472#2599930 <https://reviews.llvm.org/D97472#2599930>, @abhina.sreeskantharajan wrote:

> In D97472#2599773 <https://reviews.llvm.org/D97472#2599773>, @thopre wrote:
>
>> In D97472#2599694 <https://reviews.llvm.org/D97472#2599694>, @jhenderson wrote:
>>
>>> I haven't physically checked this (I expect it would work), but I don't think we want to add --ignore-case, in case there are other parts of the check that will be checked where case is important. For example, if the filename was part of the error message, we want it to match exactly the case of the file, as doing anything else can result in ambiguity and therefore risk for a bug to be hidden (most Windows filesystems are case insensitive, but it is possible for them to be case sensitive). Hiding the --ignore-case option behind a substitution would also be a potential for serious confusion, in my opinion, e.g. if a user explicitly wants to ignore the case themselves for other parts of their check.
>>>
>>> I've subscribed @thopre and @jdenny, who both work on FileCheck. Maybe they'll have some other ideas. If not, I think we have to detect the C++ standard library being used somehow (or at least the compiler path, which is probably a good-enough proxy).
>>
>> I cannot think of a solution on FileCheck (the regex are allowed when matching/defining a variable, not when using it) but I don't think it's the right place to address this issue. That would require anyone using the substitution to be aware of the casing difference. I think the best approach is to lower the casing of the first letter of error message on Windows. The current code already checks for win32 platform so we don't lose more generality than we what we already have. We could even abstract that behind a function (e.g. llvm_strerror).
>
> I agree adding ignore-case may not be ideal especially because we can only specify it once. If that's the case, is my previous diff on this patch better? We check
>
>   if sys.platform == 'win32' and 'MSYS' not in host_cxx 

If you mean version 3 (aka https://reviews.llvm.org/D97472?id=326671) of your diff then I personally prefer yes. It's not as elegant but it's less surprising.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97472