[riot-notifications] [RIOT-OS/RIOT] core: introduce crossfile arrays (xfa) v3 (#15002)

Marian Buschsieweke notifications at github.com
Tue Jan 5 10:56:02 CET 2021


@maribu commented on this pull request.

Looks good to me. Maybe the unit test could be spiced up a bit as suggested above, but I'm not insisting.

> +XFA_USE(xfatest_t, xfatest_use);
+XFA_USE_CONST(xfatest_t, xfatest_use_const);
+
+/* Verifying that cross file array linking is correct by iterating over an external array */
+static void test_xfa_data(void)
+{
+    unsigned n = XFA_LEN(xfatest_t, xfatest);
+    TEST_ASSERT_EQUAL_INT(2, n);
+    unsigned found = 0;
+    for (unsigned k = 0; k < n; ++k) {
+        /* we do not want to enforce the order of the data elements */
+        switch (xfatest[k].val) {
+            case 12345:
+                /* tests-core-xfa-data1.c */
+                TEST_ASSERT_EQUAL_STRING("xfatest1", xfatest[k].text);
+                ++found;

```suggestion
                TEST_ASSERT(!(found & BIT0));
                found |= BIT0;
```

Maybe it is better to use a bit mask here rather than a counter. If due to an error the value `12345` gets added two times but `0xbeef` is missing, the test would still pass.

> +{
+    unsigned n = XFA_LEN(xfatest_t, xfatest);
+    TEST_ASSERT_EQUAL_INT(2, n);
+    unsigned found = 0;
+    for (unsigned k = 0; k < n; ++k) {
+        /* we do not want to enforce the order of the data elements */
+        switch (xfatest[k].val) {
+            case 12345:
+                /* tests-core-xfa-data1.c */
+                TEST_ASSERT_EQUAL_STRING("xfatest1", xfatest[k].text);
+                ++found;
+                break;
+            case 0xbeef:
+                /* tests-core-xfa-data2.c */
+                TEST_ASSERT_EQUAL_STRING("another test string", xfatest[k].text);
+                ++found;

```suggestion
                TEST_ASSERT(!(found & BIT1));
                found |= BIT1;
```

> +        switch (xfatest[k].val) {
+            case 12345:
+                /* tests-core-xfa-data1.c */
+                TEST_ASSERT_EQUAL_STRING("xfatest1", xfatest[k].text);
+                ++found;
+                break;
+            case 0xbeef:
+                /* tests-core-xfa-data2.c */
+                TEST_ASSERT_EQUAL_STRING("another test string", xfatest[k].text);
+                ++found;
+                break;
+            default:
+                break;
+        }
+    }
+    TEST_ASSERT_EQUAL_INT(n, found);

```suggestion
    TEST_ASSERT_EQUAL_INT((1U << (n + 1)) - 1, found);
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15002#pullrequestreview-561649670
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210105/92bf123f/attachment.htm>


More information about the notifications mailing list