[PATCH] D97472: [test] Use host platform specific error message substitution in lit tests
Thomas Preud'homme via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 3 04:24:01 PST 2021
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
More information about the llvm-commits
mailing list