On Thu, 26 Aug 2021 at 03:26, 'Isabella Basso' via KUnit Development kunit-dev@googlegroups.com wrote:
Keep function signatures minimal by making common definitions static. This does not change any behavior.
This seems like an odd change; if I read it right it's changing the out-param passed to test_int_hash() to simply be static globals.
For one, it makes the code harder to read because now test_int_hash() is no longer "pure" (no global side-effects ... modulo printfs), and what was previously an out-param, is now a global.
Unfortunately this is poor style and likely to lead to hard-to-debug problems. One such problem is if suddenly you have multiple threads involved. While this is just a test and unlikely to be a problem, I would recommend not introducing global state carelessly.
An alternative common idiom, where a set of variables are always passed around to other functions, is to introduce a struct and pass a pointer to it along.
Signed-off-by: Isabella Basso isabellabdoamaral@usp.br
lib/test_hash.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/lib/test_hash.c b/lib/test_hash.c index d4b0cfdb0377..8bcc645a7294 100644 --- a/lib/test_hash.c +++ b/lib/test_hash.c @@ -23,6 +23,11 @@ #include <linux/stringhash.h> #include <linux/printk.h>
+#define SIZE 256 /* Run time is cubic in SIZE */
+static u32 string_or; /* stores or-ed string output */ +static u32 hash_or[2][33] = { { 0, } }; /* stores or-ed hash output */
These now use up memory for as long as this module is loaded, vs. before where it would only use up stack space. (For a test that's not a problem, but in non-test code it might.)
/* 32-bit XORSHIFT generator. Seed must not be zero. */ static u32 __init __attribute_const__ xorshift(u32 seed) @@ -66,7 +71,7 @@ fill_buf(char *buf, size_t len, u32 seed)
- recompile and re-test the module without rebooting.
*/ static bool __init -test_int_hash(unsigned long long h64, u32 hash_or[2][33]) +test_int_hash(unsigned long long h64) { int k; u32 h0 = (u32)h64, h1, h2; @@ -123,17 +128,15 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33]) return true; }
-#define SIZE 256 /* Run time is cubic in SIZE */
static int __init test_hash_init(void) { char buf[SIZE+1];
u32 string_or = 0, hash_or[2][33] = { { 0, } }; unsigned tests = 0; unsigned long long h64 = 0; int i, j;
string_or = 0;
That's another problem with changes like this; now the compiler has no chance to warn you in case the variable is not initialized correctly.
Also, I don't see string_or used anywhere else. Why make it global? If a later change would require that, it should say so in the commit message. But my guess is you can avoid all that by bundling everything up in a struct.
fill_buf(buf, SIZE, 1); /* Test every possible non-empty substring in the buffer. */@@ -161,7 +164,7 @@ test_hash_init(void)
string_or |= h0; h64 = h64 << 32 | h0; /* For use with hash_64 */
if (!test_int_hash(h64, hash_or))
if (!test_int_hash(h64)) return -EINVAL; tests++; } /* i */-- 2.33.0
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210826012626.1163705-3-isabell....