[PATCH] D146376: Update static_assert message for redundant cases

This revision was automatically updated to reflect the committed changes. Closed by commit rG06c6fac696e4: Update static_assert message for redundant cases (authored by Krishna-13-cyber, committed by aaron.ballman). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D146376: Update static_assert message for redundant cases

aaron.ballman added a comment. In D146376#4250814 , @xbolva00 wrote: > The patch description mixes "or" and "and" in examples, please fix it. > > SO probably you meant > > :4:1: error: static assertion failed due to requirement 'is_gitlab' > static_as

[PATCH] D146376: Update static_assert message for redundant cases

xbolva00 added a comment. So.. new diagnostic is: :4:1: error: static assertion failed due to requirement 'is_gitlab' static_assert(is_gitlab and is_weekend); ^ ~ Okay, a dev changes is_gitlab to true, compiles it again and boom, new error: static assertion failed due

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 511630. Krishna-13-cyber added a comment. 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: Thanks ag

[PATCH] D146376: Update static_assert message for redundant cases

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. 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 a

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 511181. Krishna-13-cyber added a comment. - Updated with the diagnostic messages (comments for test cases) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146376/new/ https://reviews.llvm.org/D146376 Fi

[PATCH] D146376: Update static_assert message for redundant cases

aaron.ballman added a comment. In D146376#4244355 , @Krishna-13-cyber wrote: > - 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` curre

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 510909. Krishna-13-cyber added a comment. - 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

[PATCH] D146376: Update static_assert message for redundant cases

aaron.ballman added a comment. 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}}`). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 510186. Krishna-13-cyber added a comment. - Updated diff with `git clang-format HEAD~1` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146376/new/ https://reviews.llvm.org/D146376 Files: clang/lib/Se

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added a comment. Does `git clang-format HEAD~1` not work on your system? It should only format the changed parts, not everything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146376/new/ https://reviews.llvm.org/D146376 _

[PATCH] D146376: Update static_assert message for redundant cases

aaron.ballman added inline comments. Comment at: clang/test/SemaCXX/static-assert.cpp:266-268 + static_assert(invert(true) || invert(true) || false, ""); // expected-error {{failed}} + static_assert((true && invert(true)) || false, ""); // expected-error {{failed}} + static_

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16718 void Sema::DiagnoseStaticAssertDetails(const Expr *E) { - if (const auto *Op = dyn_cast(E)) { + if (const auto *Op = dyn_cast(E);Op && Op->getOpcode() != BO_LOr) { const Expr *LHS = Op->getLH

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 509632. Krishna-13-cyber added a comment. Herald added subscribers: llvm-commits, kadircet, arphaman. Herald added a project: LLVM. - Updated patch - Added testcases for chained operators Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D146376: Update static_assert message for redundant cases

aaron.ballman added inline comments. Comment at: clang/test/SemaCXX/static-assert.cpp:262 + + static_assert(invert(true) || invert(true), ""); // expected-error {{failed}} + +1 -- please spell out more of the diagnostic wording (I see you're following a pattern

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 509589. Krishna-13-cyber edited the summary of this revision. Krishna-13-cyber added a comment. This patch aims to remove some redundant cases of static assert messages where the expansions are particularly unhelpful. In this particular patch, we hav

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16732 +// Ignore BO_LOr operators at the toplevel. +if (Op->getOpcode() == BO_LOr) + return; tbaeder wrote: > You can just drop the comment and move this into the `if` statemen

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added a comment. Make sure to include context in the patch you upload: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146376/new/ https://reviews.llvm.org/D146

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 509249. Krishna-13-cyber added a comment. 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 T

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added a comment. Remember to upload you patches with context. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16732 +// Ignore BO_LOr operators at the toplevel. +if (Op->getOpcode() == BO_LOr) + return; You can just drop the comment and move thi

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 508297. Krishna-13-cyber added a comment. 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.

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added inline comments. Comment at: clang/test/SemaCXX/static-assert.cpp:262 static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \ -// expected-note {{evaluates to 'false == true'}} -

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber added a comment. 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

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added a comment. 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. Comment at: clang/test/SemaCXX/static-assert

[PATCH] D146376: Update static_assert message for redundant cases

cjdb added inline comments. Comment at: clang/test/SemaCXX/static-assert.cpp:261-265 static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \ -// expected-note {{evaluates to 'false == true'}} /

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 507413. Krishna-13-cyber added a comment. 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! Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added a comment. Since we only handle `BinaryOperator`s 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()`. Repository: rG LLVM Gi

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber updated this revision to Diff 507113. Krishna-13-cyber added a comment. 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. Repository: rG LLVM G

[PATCH] D146376: Update static_assert message for redundant cases

cjdb added a comment. Thanks for working on this, it's much appreciated! :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. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D146376: Update static_assert message for redundant cases

tbaeder added a comment. 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. Repository:

[PATCH] D146376: Update static_assert message for redundant cases

Krishna-13-cyber created this revision. Krishna-13-cyber added reviewers: tbaeder, cjdb. Herald added a project: All. Krishna-13-cyber requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. There are some simple messages where an expansion isn't p