[PATCH] D130894: [clang] Print more information about failed static assertions

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Thank you so much for working on this! It's been on my todo list for a while and just haven't gotten around to it.

In D130894#3702166 <https://reviews.llvm.org/D130894#3702166>, @aaron.ballman wrote:

> In D130894#3701749 <https://reviews.llvm.org/D130894#3701749>, @tbaeder wrote:
>
>> For the record, the output now looks something like this:
>>
>>   test.cpp:24:1: error: static assertion failed due to requirement 'c != c'
>>   static_assert(c != c);
>>   ^             ~~~~~~
>>   test.cpp:24:17: note: expression evaluates to '('0' != '0')'
>>   static_assert(c != c);
>>                 ~~^~~~
>
> This looks hard to read to me...
>
>> test.cpp:25:1: error: static assertion failed due to requirement 'c > 'a''
>> static_assert(c > 'a');
>> ^             ~~~~~~~
>> test.cpp:25:17: note: expression evaluates to '('0' > 'a')'
>
> Same confusion here -- it took me a while to realize what's happening is that we're quoting the part outside of the parens and that's throwing me for a loop. We typically quote things that are syntax but in this case, the parens are not part of the syntax and so the surrounding quotes are catching me. Given that parens are being used as a visual delimiter and a single quote serves the same purpose, I would expect something more like:
>
>   test.cpp:25:17: note: expression evaluates to ''0' > 'a''
>   test.cpp:26:17: note: expression evaluates to '0 > 'a''
>   test.cpp:27:17: note: expression evaluates to '14 > 5'
>   test.cpp:28:17: note: expression evaluates to 'true == false'
>   test.cpp:29:17: note: expression evaluates to 'nullptr != nullptr'
>
> Adding a few more folks as reviewers to see if there is a consensus position on how to proceed.

Agreed on confusion, but `''0' > 'a''` is painful to read. Sadly, short of changing quotes to backticks (something that'd be good for non-terminal-based consumers of SARIF), I'm not sure I can suggest anything better in one line. If we can span multiple lines, which I think would improve readability:

  test.cpp:25:17: note: expression evaluates with:
    LHS: '0'
    RHS: 'a'
  test.cpp:26:17: note: expression evaluates with:
    LHS: 0
    RHS: 'a'
  test.cpp:27:17: note: expression evaluates with:
    LHS: 14
    RHS: 5
  test.cpp:28:17: note: expression evaluates with:
    LHS: true
    RHS: false
  test.cpp:29:17: note: expression evaluates with:
    LHS: nullptr
    RHS: nullptr

My assertion library follows what Catch2 and simple-test do, and uses `Actual` and `Expected`, but I'm not sure hardcoding those terms into Clang is a good idea.


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

https://reviews.llvm.org/D130894