This is an archive of the discontinued LLVM Phabricator instance.

[Lint][Verifier] NFC: Rename 'Assert*' macros to 'Check*'.
ClosedPublic

Authored by tahonermann on Mar 28 2022, 2:21 PM.

Details

Summary

The LLVM IR verifier and analysis linter defines and uses several macros in
code that performs validation of IR expectations. Previously, these macros
were named with an 'Assert' prefix. These names were misleading since the
macro definitions are not conditioned on build kind; they are defined
identically in builds that have asserts enabled and those that do not. This
was confusing since an LLVM developer might expect these macros to be
conditionally enabled as 'assert' is. Further confusion was possible since
the LLVM IR verifier is implicitly disabled (in Clang::ConstructJob()) for
builds without asserts enabled, but only for Clang driver invocations; not
for clang -cc1 invocations. This could make it appear that the macros were
not active for builds without asserts enabled, e.g. when investigating
behavior using the Clang driver, and thus lead to surprises when running
tests that exercise the clang -cc1 interface.

This change renames this set of macros as follows:

Assert -> Check
AssertDI -> CheckDI
AssertTBAA -> CheckTBAA

Diff Detail

Event Timeline

tahonermann created this revision.Mar 28 2022, 2:21 PM
tahonermann published this revision for review.Mar 28 2022, 2:36 PM
tahonermann added subscribers: erichkeane, aaron.ballman.

I've added some reviewers based on prior git history for the changed files. Please feel free to resign or nominate another reviewer!

Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 2:36 PM

I like this change, let's see what others think of it

perhaps Verify() makes a bit more sense than Check() at least for the verifier?

nikic added a comment.Mar 28 2022, 3:04 PM

My reading on the "assert" terminology here was that we don't continue executing the rest of the function if a condition fails. Basically with the same implication as ASSERT in gunit tests (vs EXPECT, which reports an error but continues running the test).

But I don't particularly care either way.

perhaps Verify() makes a bit more sense than Check() at least for the verifier?

I thought about Verify() as well, but went with Check() for consistency with CheckFailed().

aeubanks accepted this revision.Mar 29 2022, 8:42 AM
This revision is now accepted and ready to land.Mar 29 2022, 8:42 AM

Thanks for the approval @aeubanks! If no objections are raised, I'll land this Tuesday morning.

tahonermann closed this revision.Apr 5 2022, 3:50 PM

Closing per commit c54ad1360248e28a436a6a6c560ba5952d8e98cb (I failed to get "Differential Revision" added to the commit message).

llvm/lib/Analysis/Lint.cpp