This is an archive of the discontinued LLVM Phabricator instance.

Make 'static assertion failed' diagnostics point to the static assertion expression
ClosedPublic

Authored by sousajo on Apr 6 2023, 3:04 PM.

Details

Summary

"static assertion failed" pointed to the static_assert token and
then underlined the static assertion expression:

<source>:3:1: error: static assertion failed
    static_assert(false);
    ^             ~~~~~
    1 error generated.

See Godbolt: https://godbolt.org/z/r38booz59

Now one points and highlights the assertion expression.

Fixes https://github.com/llvm/llvm-project/issues/61951

Diff Detail

Event Timeline

sousajo created this revision.Apr 6 2023, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 3:04 PM
sousajo requested review of this revision.Apr 6 2023, 3:04 PM
sousajo edited the summary of this revision. (Show Details)
sousajo edited the summary of this revision. (Show Details)

The patch does not apply, so the pre-commit CI does not run.

sousajo updated this revision to Diff 511610.Apr 6 2023, 10:10 PM

just rebased

This comment was removed by sousajo.

I think the other Diag call should be changed as well:

./a.cpp:4:15: error: static assertion failed
static_assert(false);
              ^~~~~
./a.cpp:6:1: error: static assertion failed due to requirement 'true && false'
static_assert(true && false);
^             ~~~~~~~~~~~~~

Would also be nice to add a release note for this.

Also, let's ping @aaron.ballman and @cjdb, the former might want a test added. :)

I think the other Diag call should be changed as well:

./a.cpp:4:15: error: static assertion failed
static_assert(false);
              ^~~~~
./a.cpp:6:1: error: static assertion failed due to requirement 'true && false'
static_assert(true && false);
^             ~~~~~~~~~~~~~

ah sure! just did locally, will update the patch, when it has also the release note and (possibly) the test, as to not waste CI resources :)

Would also be nice to add a release note for this.

Also, let's ping @aaron.ballman and @cjdb, the former might want a test added. :)

Okidoki, would also be nice to get a pointer to a similar test, so I can take a look into it, and meanwhile Ill try to find some example for the release note.

sousajo updated this revision to Diff 511912.Apr 8 2023, 11:34 AM

Added two test cases, and the release note.

Just realized that in the case:

static_assert(
true && 
false);

We will now point to:

test.cpp:5:1: error: static assertion failed due to requirement 'true && false'
true && 
^~~~~~~

Which seems a bit off. gcc points to the conditional operator instead:

https://godbolt.org/z/MjsEs7eW6

I think this is generally a shortcoming in clang, we never emit more than one line of code, even if we should.

Ah, there is -fcared-diagnostics-max-lines as a cc1 option: https://godbolt.org/z/Y4dW35v7M

Ah, there is -fcared-diagnostics-max-lines as a cc1 option: https://godbolt.org/z/Y4dW35v7M

Ah oki, using that option:

test.cpp:5:1: error: static assertion failed due to requirement 'true && false || false'
true && 
^~~~~~~
false
~~~~~
|| false);
~~~~~~~~

Shows the whole expression :)

tbaeder added inline comments.Apr 9 2023, 10:05 AM
clang/lib/Sema/SemaDeclCXX.cpp
16822–16824
16832
16835–16836
sousajo updated this revision to Diff 512027.Apr 9 2023, 11:34 AM
  • change AssertExpr->getSourceRange().getBegin() calls to AssertExpr->getBeginLoc()
  • same for InnerCond->getSourceRange().getBegin()
  • add a couple more tests
sousajo marked 3 inline comments as done.Apr 9 2023, 11:34 AM

Does it look fine now ?

tbaeder added a comment.EditedApr 12 2023, 8:56 AM

I think so, I'm just waiting for @aaron.ballman or @cjdb do chime in. Also, with https://reviews.llvm.org/D147875 in the mix, it might be useful to point to the static_assert token in some way? But it's not important I think, part of the expression is probably always on the same line as the static_assert token.

cjdb accepted this revision.Apr 12 2023, 9:41 AM

LGTM from a diagnostics perspective. Having some pointer to the static_assert would be nice in situations where the expression is far removed from the start of the static_assert keyword (probably due to a long comment), but that would result in a note, and that isn't really worth it IMO.

clang/test/SemaCXX/static-assert.cpp
302 ↗(On Diff #512027)

It's worth being mindful of D146376, which might impact this message.

This revision is now accepted and ready to land.Apr 12 2023, 9:41 AM
sousajo added inline comments.Apr 12 2023, 9:55 AM
clang/test/SemaCXX/static-assert.cpp
302 ↗(On Diff #512027)

Ill rebase to be sure we have it

sousajo marked an inline comment as not done.EditedApr 12 2023, 9:57 AM

Also agree, imho this is better now and will be fine most of time

aaron.ballman accepted this revision.Apr 12 2023, 10:27 AM

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

clang/docs/ReleaseNotes.rst
219 ↗(On Diff #512027)

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

Sure I can add it later today. Then one of you would have to land it for me ^^

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

Sure I can add it later today. Then one of you would have to land it for me ^^

Thank you for letting us know -- what name and email address would you like us to use for patch attribution?

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

Sure I can add it later today. Then one of you would have to land it for me ^^

Thank you for letting us know -- what name and email address would you like us to use for patch attribution?

Name: Jorge Pinto Sousa
Email: <jorge.pinto.sousa@proton.me>

^^

sousajo updated this revision to Diff 513001.Apr 12 2023, 4:06 PM

Rebased and fix wording of the release note

windows checks are failing, but looks like it is unrelated to this change

This revision was landed with ongoing or failed builds.Apr 13 2023, 5:15 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 5:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript