This is an archive of the discontinued LLVM Phabricator instance.

Improve the formatting of static_assert messages
ClosedPublic

Authored by cor3ntin on Jun 29 2022, 11:44 AM.

Details

Summary

Display 'static_assert failed: message' instead of
'static_assert failed "message"' to be consistent
with other implementations and be slightly more
readable.

Diff Detail

Event Timeline

cor3ntin created this revision.Jun 29 2022, 11:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 11:44 AM
Herald added a subscriber: wenlei. · View Herald Transcript
cor3ntin requested review of this revision.Jun 29 2022, 11:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 29 2022, 11:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision.Jun 29 2022, 12:32 PM
philnik added 1 blocking reviewer(s): aaron.ballman.
philnik added a subscriber: philnik.

LGTM from the libc++ side of things.

aaron.ballman accepted this revision.Jun 30 2022, 6:20 AM

I agree with the changes in principle, but it looks like the libc++ precommit CI builder is failing with a bunch of failures... but those failures look like the precommit CI isn't testing the Clang built by the patch?

error: 'error' diagnostics expected but not seen:
  File * Line * (directive at /home/libcxx-builder/.buildkite-agent/builds/5d14e636cf69-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/rand/rand.eng/rand.eng.lcong/params.fail.cpp:22): static_assert failed due to requirement '1ULL == 0 || 1ULL < 1ULL': linear_congruential_engine invalid parameters
  File * Line * (directive at /home/libcxx-builder/.buildkite-agent/builds/5d14e636cf69-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/rand/rand.eng/rand.eng.lcong/params.fail.cpp:24): static_assert failed due to requirement '1ULL == 0 || 1ULL < 1ULL': linear_congruential_engine invalid parameters
  File * Line * (directive at /home/libcxx-builder/.buildkite-agent/builds/5d14e636cf69-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/rand/rand.eng/rand.eng.lcong/params.fail.cpp:27): static_assert failed due to requirement 'is_unsigned<int>::value': _UIntType must be unsigned type
error: 'error' diagnostics seen but not expected:
  Line 218: static_assert failed due to requirement '1ULL == 0 || 1ULL < 1ULL' "linear_congruential_engine invalid parameters"
  Line 217: static_assert failed due to requirement '1ULL == 0 || 1ULL < 1ULL' "linear_congruential_engine invalid parameters"
  Line 219: static_assert failed due to requirement 'is_unsigned<int>::value' "_UIntType must be unsigned type"
6 errors generated.

So I *think* this LG, but am not certain what will happen when you land it. I think the precommit CI failures are not failures that will happen when testing against the just-built clang.

This revision is now accepted and ready to land.Jun 30 2022, 6:20 AM
philnik requested changes to this revision.Jun 30 2022, 6:28 AM

Sorry I missed that. You have to changes the libc++ tests to regex checks to allow both error-styles. This can then be removed once we drop support for LLVM 14 (so after the release of LLVM 16).

This revision now requires changes to proceed.Jun 30 2022, 6:28 AM
cor3ntin updated this revision to Diff 441439.Jun 30 2022, 9:44 AM

Pushing for CI

Mordante accepted this revision.Jun 30 2022, 10:13 AM
Mordante added a subscriber: Mordante.

So I *think* this LG, but am not certain what will happen when you land it. I think the precommit CI failures are not failures that will happen when testing against the just-built clang.

Most Linux runner use the apt.llvm.org nightly builds and are updated on an irregular interval. The Bootstrapping build will use the just-built Clang. So when the CI is green all should be fine.

I wanted to suggest a regex, but I see the latest patch already does that.

Thanks for improving the readability of the diagnostics! LGTM when the CI passes.

cor3ntin updated this revision to Diff 441448.Jun 30 2022, 10:17 AM

Missing -re in a few places

@philnik There seems to be an unrelated clang-format issue but otherwise the build looks fine now.

philnik accepted this revision.Jun 30 2022, 2:52 PM

LGTM. (The clang-format if more of a suggestion than anything else currently)

This revision is now accepted and ready to land.Jun 30 2022, 2:52 PM
This revision was landed with ongoing or failed builds.Jun 30 2022, 2:59 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!