This is an archive of the discontinued LLVM Phabricator instance.

Update static_assert message for redundant cases
ClosedPublic

Authored by Krishna-13-cyber on Mar 18 2023, 11:53 PM.

Details

Summary

There are some simple messages where an expansion isn't particularly helpful or needed. We aim to eliminate expansions that don't add much value for user (this will give us slightly more room or prioritise to have longer diagnostics elsewhere).

The test case for which it has been tested:

constexpr auto is_gitlab = false;
constexpr auto is_weekend = false;
static_assert(is_gitlab or is_weekend);

Previous warning/error message:

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab || is_weekend'
static_assert(is_gitlab or is_weekend);
^             ~~~~~~~~~~~~~~~~~~~~~~~
<source>:4:25: note: expression evaluates to 'false || false'
static_assert(is_gitlab or is_weekend);
              ~~~~~~~~~~^~~~~~~~~~~~~

Currrent warning/error message:

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab and is_weekend);
^             ~~~~~~~~~

This patch aims to remove some redundant cases of static assert messages where the expansions are particularly unhelpful. In this particular patch, we have ignored the printing of diagnostic warnings for binary operators with logical OR operations.
This is done to prioritise and prefer to emit longer diagnostic warnings for more important concerns elsewhere.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2023, 11:53 PM
Krishna-13-cyber requested review of this revision.Mar 18 2023, 11:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2023, 11:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Looks like you're just removing the output altogether, so that won't work.

Try running ninja check-clang-semacxx and see all the test failures you (should) get. For a proper patch, it would also be good if you add a test case for the case you're fixing.

cjdb added a comment.Mar 20 2023, 10:44 AM

Thanks for working on this, it's much appreciated!

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab and is_weekend);
^             ~~~~~~~~~

The arrow seems a bit off here.

Krishna-13-cyber updated this revision to Diff 507113.EditedMar 21 2023, 1:59 PM

I have updated the patch without removing the whole of the previous block.I have added a condition to find the case where we don't need a diagnostic message to be printed or warned.

It passes all test cases without any failure or regression!

Since we only handle BinaryOperators in the toplevel assertion expression, I think it should just be safe to never diagnose for them if it's an BO_LOr opcode, so you should be able to just check for that in DiagnoseStaticAssertDetails().

Have worked on the top level with Boolean literals unlike the previous diffs.I have updated the test case as well with this new upcoming change.

Thanks!

cjdb added inline comments.Mar 22 2023, 10:03 AM
clang/test/SemaCXX/static-assert.cpp
261–265 ↗(On Diff #507413)

We should also have a test for conjunctions, disjunctions, and negations.

I think just checking that the toplevel binary operator is a logical or should be enough? Since that only fails if both operands evaluate to false, so we don't need to print the "false or false" diagnostic.

clang/test/SemaCXX/static-assert.cpp
262 ↗(On Diff #507413)

This diagnostic should be kept. From looking at the condition, it is not obvious what the two functions evaluate to.

The above Binary operator test case says that there are two Boolean expressions,UsefulToPrintExpr says we can avoid to print these expressions as they are don't need a note and are understandable.

So if we go by this we will have to remove the note.By this we are removing note for boolean literals as well as expressions.It will be nice to make it generic rather than specifically target cases of false || false , false && false. As the warning note print out the boolean values which can be avoided giving preference to the other diagnostics in terms of boolean literals and expressions.

Yes, I making a new diff for test cases of conjunction and disjunction as well.

clang/test/SemaCXX/static-assert.cpp
261–265 ↗(On Diff #507413)

Yes, I making a new diff for test cases of conjunction and disjunction as well.

262 ↗(On Diff #507413)

The above Binary operator test case says that there are two Boolean expressions,UsefulToPrintExpr says we can avoid to print these expressions as they are don't need a note and are understandable.

So if we go by this we will have to remove the note.By this we are removing note for boolean literals as well as expressions.It will be nice to make it generic rather than specifically target cases of false || false , false && false. As the warning note print out the boolean values which can be avoided giving preference to the other diagnostics in terms of boolean literals and expressions.

tbaeder added inline comments.Mar 24 2023, 8:52 AM
clang/test/SemaCXX/static-assert.cpp
262 ↗(On Diff #507413)

I think you are confusing boolean expressions with boolean literals.

What is the problem with fixing this issue by simply ignoring toplevel BO_LOr binary operators?

Updated with the new test cases and implemented the logic suggested for ignoring the BO_LOr operators at the top level. My sincere apologies @tbaeder that I could not place this logic at the first instance.

Thanks!

Remember to upload you patches with context.

clang/lib/Sema/SemaDeclCXX.cpp
16732

You can just drop the comment and move this into the if statement above, e..g if (...; BO && BO->getOpcode() != BO_Lor).

clang/test/SemaCXX/static-assert.cpp
262 ↗(On Diff #508297)

That comment doesn't make a lot of sense?

263 ↗(On Diff #508297)

Remove the backslash

269 ↗(On Diff #508297)

Neither this one.

270 ↗(On Diff #508297)

Remove the backslash

Krishna-13-cyber updated this revision to Diff 509249.EditedMar 29 2023, 1:47 AM

I have removed the redundant comments and have moved the if condition above with the ongoing conditional statement (to avoid printing obvious expressions).

  • Updated static_assert warning message
  • Added Test cases

Make sure to include context in the patch you upload: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

clang/lib/Sema/SemaDeclCXX.cpp
16732

I was talking about the one that checks that E is a BinaryOperator.

Krishna-13-cyber edited the summary of this revision. (Show Details)

This patch aims to remove some redundant cases of static assert messages where the expansions are particularly unhelpful. In this particular patch, we have ignored the printing of diagnostic warnings for binary operators with logical OR operations.
This is done to prioritise and prefer to emit longer diagnostic warnings for more important concerns elsewhere.

aaron.ballman added inline comments.
clang/test/SemaCXX/static-assert.cpp
262 ↗(On Diff #509589)

+1 -- please spell out more of the diagnostic wording (I see you're following a pattern elsewhere in the file, but we should have enough diagnostic wording to know what the diagnostic is.)

It's hard for me to tell from this review because the patch was not uploaded with context (next time, please use -U9999 when making the patch diff so that this context exists), but is there test coverage showing what happens when the operators are chained together? e.g.,

static_assert(inverted(true) || invert(true) || false, "");
static_assert((true && invert(true)) || false, "");
static_assert(true && inverted(false) && inverted(true), "");
  • Updated patch
  • Added testcases for chained operators
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 5:52 AM
tbaeder added inline comments.Mar 30 2023, 6:41 AM
clang/lib/Sema/SemaDeclCXX.cpp
16718

This line is over 80 colums now, remember to format it: https://llvm.org/docs/Contributing.html#format-patches

aaron.ballman added inline comments.Mar 30 2023, 6:52 AM
clang/test/SemaCXX/static-assert.cpp
266–268 ↗(On Diff #509632)

We really need to see more of the diagnostic to know whether the behavior is reasonable or not.

I was thinking we'd have given a note as to why the failure occurred, but I see now that the current behavior doesn't give a note either. Not that I think this is related to your patch, but the current behavior seems a bit surprising: https://godbolt.org/z/erPGdsanK Note how the last diagnostic points to exactly where the failure comes from but the first two use the entire expression as the failure.

llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
26 ↗(On Diff #509632)

This seems like an unrelated change.

  • Updated with clang-formatting
  • Removed the redundant from the diff

Actually I had done that at the first place clang-format -i file.cpp but that seems to make a lot of changes and the diff becomes really big. I was a bit uncertain on that due to which I did not use it.
I have renewed the diff.
Thanks!

Does git clang-format HEAD~1 not work on your system? It should only format the changed parts, not everything.

  • Updated diff with git clang-format HEAD~1

The changes need a release note and please do add more text to the expected-error comments so we can see what the diagnostic actually produces (instead of just {{failed}}).

  • Updated with release note
  • I had tried adding more text to the expected-error but it already gives a diagnostic of static assertion failed due to requirement currently. If I try additions to {{failed}} then we get error diagnostics expected but not seen.
  • Updated with release note
  • I had tried adding more text to the expected-error but it already gives a diagnostic of static assertion failed due to requirement currently. If I try additions to {{failed}} then we get error diagnostics expected but not seen.

Ah, sorry for being unclear! We didn't mean add an additional expected-error comment, but to modify the ones you have. e.g.,

Currently:

static_assert(true && false, ""); // expected-error {{failed}}

Changes to:

static_assert(true && false, ""); // expected-error {{static assertion failed due to requirement 'true && false'}}

(or whatever the actual expected full text of the diagnostic is.)

This helps the reviewers to make sure that the diagnostic behavior isn't regressing in surprising ways but pass all our tests because we're only looking for the word failed and considering that passing.

  • Updated with the diagnostic messages (comments for test cases)
aaron.ballman accepted this revision.Apr 6 2023, 9:07 AM

LGTM aside from a small nit with the release notes. Do you need someone to land this on your behalf? If so, please rebase the patch so it applies cleanly to trunk and upload it again, and let me know what name and email address you would like used for patch attribution.

clang/docs/ReleaseNotes.rst
183 ↗(On Diff #511181)
This revision is now accepted and ready to land.Apr 6 2023, 9:07 AM

Thanks a lot everyone for helping me out with my first patch. I have uploaded the patch again after rebase.
Yes, It would be great if you could land it on my behalf.
Name: Krishna Narayanan
Email: <krishnanarayanan132002@gmail.com>

Thanks again!

xbolva00 added a subscriber: xbolva00.EditedApr 7 2023, 1:41 AM

The patch description mixes "or" and "and" in examples, please fix it.

SO probably you meant

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab or is_weekend);
^             ~~~~~~~~~

The patch description mixes "or" and "and" in examples, please fix it.

SO probably you meant

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab or is_weekend);
^             ~~~~~~~~~

Good catch, I'll fix that when landing.

This revision was automatically updated to reflect the committed changes.