diff --git a/common/check.h b/common/check.h index 1db43681c7d0..33386bcf5aa3 100644 --- a/common/check.h +++ b/common/check.h @@ -20,9 +20,9 @@ namespace Carbon { // example in a template argument list: // CARBON_CHECK((inst.IsOneOf()), // "Unexpected inst {0}", inst); -#define CARBON_CHECK(condition, ...) \ - (condition) ? (void)0 \ - : CARBON_INTERNAL_CHECK(condition __VA_OPT__(, ) __VA_ARGS__) +#define CARBON_CHECK(condition, ...) \ + CARBON_INTERNAL_CHECK_CONDITION(condition) \ + ? (void)0 : CARBON_INTERNAL_CHECK(condition __VA_OPT__(, ) __VA_ARGS__) // DCHECK calls CHECK in debug mode, and does nothing otherwise. #ifndef NDEBUG @@ -35,9 +35,9 @@ namespace Carbon { // prefix it with a short-circuit operator, and we still emit the (dead) call to // the check implementation. But we use a special implementation that reduces // the compile time cost. -#define CARBON_DCHECK(condition, ...) \ - (true || (condition)) \ - ? (void)0 \ +#define CARBON_DCHECK(condition, ...) \ + (true || CARBON_INTERNAL_CHECK_CONDITION(condition)) \ + ? (void)0 \ : CARBON_INTERNAL_DEAD_DCHECK(condition __VA_OPT__(, ) __VA_ARGS__) #endif diff --git a/common/check_internal.h b/common/check_internal.h index 3a97adf95730..83cba46e127d 100644 --- a/common/check_internal.h +++ b/common/check_internal.h @@ -10,6 +10,24 @@ namespace Carbon::Internal { +// Evaluates a condition in a CHECK. This diagnoses if the condition evaluates +// to the constant `true` or `false`. +[[clang::always_inline]] constexpr bool +// Trailing GNU function attributes are incompatible with trailing return types. +// Filed as https://github.com/llvm/llvm-project/issues/118697 +// NOLINTNEXTLINE(modernize-use-trailing-return-type) +CheckCondition(bool condition) + __attribute__((diagnose_if(condition, + "CHECK condition is always true; replace with " + "static_assert if this is intended", + "error"))) + __attribute__((diagnose_if(!condition, + "CHECK condition is always false; replace with " + "CARBON_FATAL if this is intended", + "error"))) { + return condition; +} + // Implements the check failure message printing. // // This is out-of-line and will arrange to stop the program, print any debugging @@ -54,6 +72,13 @@ template bool { return true; } +auto AlwaysFalse() -> bool { return false; } + +TEST(CheckTest, CheckTrue) { CARBON_CHECK(AlwaysTrue()); } TEST(CheckTest, CheckFalse) { - ASSERT_DEATH({ CARBON_CHECK(false); }, - "\nCHECK failure at common/check_test.cpp:\\d+: false\n"); + ASSERT_DEATH({ CARBON_CHECK(AlwaysFalse()); }, + R"( +CHECK failure at common/check_test.cpp:\d+: AlwaysFalse\(\) +)"); } TEST(CheckTest, CheckFalseHasStackDump) { - ASSERT_DEATH({ CARBON_CHECK(false); }, "\nStack dump:\n"); + ASSERT_DEATH({ CARBON_CHECK(AlwaysFalse()); }, "\nStack dump:\n"); } TEST(CheckTest, CheckTrueCallbackNotUsed) { @@ -26,13 +33,15 @@ TEST(CheckTest, CheckTrueCallbackNotUsed) { called = true; return "called"; }; - CARBON_CHECK(true, "{0}", callback()); + CARBON_CHECK(AlwaysTrue(), "{0}", callback()); EXPECT_FALSE(called); } TEST(CheckTest, CheckFalseMessage) { - ASSERT_DEATH({ CARBON_CHECK(false, "msg"); }, - "\nCHECK failure at common/check_test.cpp:.+: false: msg\n"); + ASSERT_DEATH({ CARBON_CHECK(AlwaysFalse(), "msg"); }, + R"( +CHECK failure at common/check_test.cpp:.+: AlwaysFalse\(\): msg +)"); } TEST(CheckTest, CheckFalseFormattedMessage) { @@ -40,15 +49,17 @@ TEST(CheckTest, CheckFalseFormattedMessage) { std::string str = "str"; int i = 1; ASSERT_DEATH( - { CARBON_CHECK(false, "{0} {1} {2} {3}", msg, str, i, 0); }, - "\nCHECK failure at common/check_test.cpp:.+: false: msg str 1 0\n"); + { CARBON_CHECK(AlwaysFalse(), "{0} {1} {2} {3}", msg, str, i, 0); }, + R"( +CHECK failure at common/check_test.cpp:.+: AlwaysFalse\(\): msg str 1 0 +)"); } TEST(CheckTest, CheckOutputForms) { const char msg[] = "msg"; std::string str = "str"; int i = 1; - CARBON_CHECK(true, "{0} {1} {2} {3}", msg, str, i, 0); + CARBON_CHECK(AlwaysTrue(), "{0} {1} {2} {3}", msg, str, i, 0); } TEST(CheckTest, Fatal) { @@ -67,5 +78,16 @@ TEST(ErrorTest, FatalNoReturnRequired) { "\nFATAL failure at common/check_test.cpp:.+: msg\n"); } +// Detects whether `CARBON_CHECK(F())` compiles. +template +concept CheckCompilesWithCondition = requires { CARBON_CHECK(F()); }; + +TEST(CheckTest, CheckConstantCondition) { + EXPECT_TRUE(CheckCompilesWithCondition<[] { return AlwaysTrue(); }>); + EXPECT_TRUE(CheckCompilesWithCondition<[] { return AlwaysFalse(); }>); + EXPECT_FALSE(CheckCompilesWithCondition<[] { return true; }>); + EXPECT_FALSE(CheckCompilesWithCondition<[] { return false; }>); +} + } // namespace } // namespace Carbon diff --git a/common/raw_hashtable_metadata_group.h b/common/raw_hashtable_metadata_group.h index cc2b9d01079b..e15d7aeb4176 100644 --- a/common/raw_hashtable_metadata_group.h +++ b/common/raw_hashtable_metadata_group.h @@ -366,9 +366,14 @@ class MetadataGroup : public Printable { auto Store(uint8_t* metadata, ssize_t index) const -> void; // Clear a byte of this group's metadata at the provided `byte_index` to the - // empty value. Note that this must only be called when `FastByteClear` is - // true -- in all other cases users of this class should arrange to clear - // individual bytes in the underlying array rather than using the group API. + // empty value. + // + // Note that this must only be called when `FastByteClear` is true -- in all + // other cases users of this class should arrange to clear individual bytes in + // the underlying array rather than using the group API. This is checked by a + // static_assert, and the function is templated so that it is not instantiated + // in the cases where it would not be valid. + template auto ClearByte(ssize_t byte_index) -> void; // Clear all of this group's metadata bytes that indicate a deleted slot to @@ -533,9 +538,12 @@ inline auto MetadataGroup::Store(uint8_t* metadata, ssize_t index) const CARBON_DCHECK(0 == std::memcmp(metadata + index, &metadata_bytes, Size)); } +template inline auto MetadataGroup::ClearByte(ssize_t byte_index) -> void { - CARBON_DCHECK(FastByteClear, "Only use byte clearing when fast!"); - CARBON_DCHECK(Size == 8, "The clear implementation assumes an 8-byte group."); + static_assert(!IsCalled || FastByteClear, + "Only use byte clearing when fast!"); + static_assert(!IsCalled || Size == 8, + "The clear implementation assumes an 8-byte group."); metadata_ints[0] &= ~(static_cast(0xff) << (byte_index * 8)); } diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index fa2ab103d404..907df792a5d1 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -1351,7 +1351,7 @@ auto TypeChecker::ArgumentDeduction::Deduce(Nonnull param, } case Value::Kind::MixinPseudoType: case Value::Kind::TypeOfMixinPseudoType: - CARBON_CHECK(false, "Type expression must not contain Mixin types"); + CARBON_FATAL("Type expression must not contain Mixin types"); } }