On Fri, Jan 28, 2022 at 1:21 PM Brendan Higgins brendanhiggins@google.com wrote:
On Thu, Jan 27, 2022 at 4:52 PM Daniel Latypov dlatypov@google.com wrote:
All the operands should be tagged `const`. We're only assigning them to variables so that we can compare them (e.g. check if left == right, etc.) and avoid evaluating expressions multiple times.
There's no need for them to be mutable.
Agreed.
Also rename the helper variable `loc` to `__loc` like we do with `__assertion` and `__strs` to avoid potential name collisions with user code.
Probably not necessary since we create a new code block (we are inside of an if-statement, do-while-loop, etc), but I don't really care either way.
You're totally right that this doesn't matter with our current macros.
given int loc = 42; KUNIT_EXPECT_TRUE(test, loc); KUNIT_EXPECT_EQ(test, loc, 42);
becomes do { if (__builtin_expect(!!(!(!!(loc) == !!true)), 0)) { /* we don't use the operands in here, so `loc` is fine */ static const struct kunit_loc loc = { .file = "lib/kunit/kunit-example-test.c", .line = 25 }; ... do { typeof(loc) __left = (loc); typeof(42) __right = (42); do { /* We never reference the expression again, so `loc` is fine */ if (__builtin_expect(!!(!(__left == __right)), 0)) { static const struct kunit_loc loc = { .file = "lib/kunit/kunit-example-test.c", .line = 24 };
But reminder: this was *not* the case until very recently. Sau we didn't have my earlier patch to move the `if(!(passed))` check into the macro. Then we'd have issues, e.g. ../lib/kunit/kunit-example-test.c: In function ‘example_simple_test’: ../include/kunit/test.h:828:26: error: wrong type argument to unary exclamation mark 828 | !!(condition) == !!expected_true, \ | ... ../lib/kunit/kunit-example-test.c:25:9: note: in expansion of macro ‘KUNIT_EXPECT_TRUE’ 25 | KUNIT_EXPECT_TRUE(test, loc); | ^~~~~~~~~~~~~~~~~
So being defensive here lets us change up our implementation more freely.
Signed-off-by: Daniel Latypov dlatypov@google.com
Reviewed-by: Brendan Higgins brendanhiggins@google.com