add static_check_equal for easier to read compiler errors #19531

I find myself reaching for this function all the time and re-implementing it. Maybe it makes sense to add to util/check.h and eventually replace uses of static_assert with this to save contributor time when diagnosing failing static_asserts.

The benefit of

static_check_equal(1, 2)

is that it prints 1 and 2 in the compiler error.

./util/check.h: In instantiation of ‘static_check_equal_struct<T1, T2, value_1, value_2>::static_check_equal_struct() [with T1 = int; T2 = int; T1 value_1 = 1; T2 value_2 = 2]’:
txmempool.cpp:29:5:   required from here
./util/check.h:63:31: error: static assertion failed: Equality check failed, value_1 != value_2.
   63 |         static_assert(value_1 == value_2, "Equality check failed, value_1 != value_2.");

Compare to:

static_assert(sizeof(int) == sizeof(size_t))

which just gives

txmempool.cpp:29:31: error: static assertion failed
   29 |     static_assert(sizeof(int) == sizeof(size_t));

is that you don't actually get to see what size T and T2 actually are, which can make it annoying to debug what went wrong, so then you end up guessing and recompiling or having to run the code to print the size out.

Concept ACK

Could introduce an usage of static_check_equal(…, …) too to make it technically in use? :)

Good point -- maybe we want to permit passing in an optional string so it matches existing static assert API?

Tested Concept ACK.

Perhaps place it somewhere that is always included to avoid the need to add #include <util/check.h> in the file where you want to use it.

Concept ACK

Concept ACK (but it'd be nice to have one example use at least in the PR?)

Perhaps place it somewhere that is always included to avoid the need to add #include <util/check.h> in the file where you want to use it.

Dunno. Generally I like having independent things in different files and being explicit about it, instead of having "hotspots". But then I don't know how often this is going to be used.

I'd been kinda stuck figuring out how to pass the message to the static_assert because the message param has to be a literal string and string literals are forbidden for templates until later c++. But I realized it's not a problem to just ignore the message string as long as we have it in the source, it will be visible in the callsite error. And then I got stuck figuring out how to make it support common binary predicates without too much repetition when std::less and co aren't constexpr till after c++14. But I realized I can just pass the operator directly in the macro.

Thus the updates, which have support for ==, !=, >, <, >=, <= and message or no message.

I also added an example of where this is handy -- reviewers may wish to test by compiling this branch and tweaking the arguments +/- 1 to see that it fails, and removing the message.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

review ACK 8047fed26dd5077dacd490b35e481c77cb00722a

Tested ACK 8047fed26dd5077dacd490b35e481c77cb00722a

Took me quite a bit to figure out how these preprocessor macros are constructed in a way to magically work both with and without message passed. Nice! Tested by modifying the introduced example in random.cpp to fail with various operators and removing the message (third parameter), works as expected.

i guess this is mergeable then? hooray for compile time errors :)

Approach ACK.

You could simplify this a bit:

template <auto value_1, auto value_2, bool b>     // C++17 lets the type params be implicit
struct static_check_struct {
    static_assert(b);   // don't need to put static_assert in a function, don't need a dummy message
};

#define static_check(f, s1, s2, ...) static_check_struct<s1, s2, s1 f s2>()
#define static_check_macro(...) static_check(__VA_ARGS__, "") // accept 0 or more comments instead of 0 or 1

#define static_check_equal(...) static_check_macro(==, __VA_ARGS__)

With those changes, clang's errors look like:

In file included from random.cpp:22:
./util/check.h:100:5: error: static_assert failed
    static_assert(b);
    ^             ~
random.cpp:413:9: note: in instantiation of template class 'static_check_struct<64UL, 65UL, false>' requested here
        static_check_equal(sizeof(buf), 1+CSHA512::OUTPUT_SIZE, "Buffer needs to have hasher's output size");
        ^

(plus the "expanded from macro" notes)

Feel free to pick this branch and do the simplifications if you'd like, It's not really clear that even with those changes anything would lead to merge so I'm unlikely to spend time on it.

Hmm, I was testing this and it looks like it doesn't work outside a function or class scope.

I get:

error: expected unqualified-id
static_check_equal(1, 1);
^

A hacky workaround for that might be to have a static constexpr bool TRUE{true}; dummy member in the struct and then wrap everything into another layer of static_assert (to ensure a two-way drop in replacement for static_assert in all scopes).

#define static_check(f, s1, s2, ...) static_assert(static_check_struct<s1, s2, s1 f s2>{}.TRUE)

A hacky workaround for that might be to have a static constexpr bool TRUE{true}; dummy member in the struct and then wrap everything into another layer of static_assert (to ensure a two-way drop in replacement for static_assert in all scopes).

template <auto value_1, auto value_2, bool b>
struct static_check_struct {
    static constexpr bool value = b;
}; 
#define static_check_macro_msg(f, s1, s2, msg) static_assert(static_check_struct<s1, s2, s1 f s2>::value)

seems to work fine:

random.cpp:409:5: error: static_assert failed due to requirement 'static_check_struct<7, 8, false>::value'
    static_check_equal(7, 8, "whatwhat");
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

random.cpp:415:9: error: static_assert failed due to requirement 'static_check_struct<64UL, 65UL, false>::value'
        static_check_equal(sizeof(buf), 1+CSHA512::OUTPUT_SIZE, "Buffer needs to have hasher's output size");
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

You could pass the msg argument into static_assert that way too, if desired.

Are you still working on this?

Closing for now. This can be reopened or a new pull can be opened