unittests: fix array length error on OSX by smlng · Pull Request #6333 · RIOT-OS/RIOT

smlng · 2017-01-12T16:23:00Z

fixes multiple errors in tests-base64 unittest on OSX, like the following one:

/RIOT/tests/unittests/tests-base64/tests-base64.c:307:26: error: variable length array folded to constant array as an
      extension [-Werror,-Wgnu-folding-constant]
    unsigned char buffer[buffer_size];

Kijewski · 2017-01-12T16:25:05Z

tests/unittests/tests-base64/tests-base64.c

 static void test_base64_08_encode_16_bytes(void)
 {
-    const int buffer_size = 16;
+    int buffer_size = 16;


Hm, VLAs should not be used if you can help it. I would rather use enum { buffer_size = 16 };.

yeah, read about that solution, too. But found this a quicker fix, by removing const. Anyway, there are more of these to fix, so I can adapt to the enum - on the other hand its a test not shipped core code of RIOT

Kijewski · 2017-01-12T17:12:23Z

tests/unittests/tests-base64/tests-base64.c

        buffer[i] = 'a';
    }

-    const size_t expected_out_size = 24;


This one, too. Probably the compiler wouldn't use VLAs in here anyway, because it should be able to deduce that the length is constant, but if it does use VLAs then that it would be a needless waste of stack and code size.

smlng · 2017-01-13T20:05:46Z

@Kijewski are you okay with these changes, please note that this PR only intends to fix compile errors revealed on macOS using clang. This is not about enhancing this (or other) tests in general, there might be other things to improve here, but that I leave for another PR

miri64 · 2017-01-14T11:22:03Z

tests/unittests/tests-base64/tests-base64.c

 {
-    const int buffer_size = 16;
+    enum { buffer_size = 16 };
    unsigned char buffer[buffer_size];


Why these are initialized as they are is a little bit strange (one could just use a macro for that), but let's fix this in a separate PR.

The Idea was to directly see the set size of the the buffer, instead of the need scrolling there and forth to check it.
It helped me a lot during the initial developing.

BytesGalore · 2017-01-16T09:40:56Z

tests/unittests/tests-base64/tests-base64.c

 static void test_base64_08_encode_16_bytes(void)
 {
-    const int buffer_size = 16;
+    enum { buffer_size = 16 };


hmmm, don't know if I like this approach.
I won't object, but at least for me its uncommon to initialize arrays with an enum value.

@smlng, ~~can you elaborate?~~ can you leave a comment about this unusual initialization?

was suggested by @Kijewski and after some lookup I also found in several blogs online that this is (besides using magic numbers or #define) the way to go.

smlng · 2017-01-16T09:44:00Z

hey all, this PR is about fixing compiler errors to make the tests macOS compatible. This is not about coding style or structure!

Regarding the usage of enum it was suggested by @Kijewski and I also found it in several blogs. Another option are #define macros as suggested by @miri64.

But still, which way ever the point is to make it compile and work on macOS by calming clang ...

miri64 · 2017-01-16T12:36:07Z

hey all, this PR is about fixing compiler errors to make the tests macOS compatible. This is not about coding style or structure!

Well the weird coding style of these tests caused the problem with OSX compatibility in the first place IMHO, so fixing the style could also help fixing the problem. The #define way is more in line with the rest of the code, but to not have 1000 defines and be in line with @BytesGalore's argument I'm also willing to accept literals, which IMHO are okay in tests anyways.

smlng · 2017-01-16T12:42:21Z

@miri64 using literals by just removing const was my first move here, but was opposed by @Kijewski - my argument back then went along similar lines as yours, stating: these are just tests and not core code we ship.

So any consensus or BCP to solve this?

miri64 · 2017-01-16T13:38:37Z

@miri64 using literals by just removing const was my first move here, but was opposed by @Kijewski - my argument back then went along similar lines as yours, stating: these are just tests and not core code we ship.

Erm... you made constants to variables not literals. What I suggested was something in the likes of

static void test_base64_08_encode_16_bytes(void)
{
    unsigned char buffer[16];
    for (int i = 0; i < 16; ++i) {
        buffer[i] = 'a';
    }

    size_t element_base64_out_size = 24;
    unsigned char element_base64_out[24];

    size_t required_out_size = 0;
    int ret = base64_encode(buffer, buffer_size, \
                            element_base64_out, &required_out_size);

    TEST_ASSERT_EQUAL_INT(BASE64_ERROR_BUFFER_OUT_SIZE, ret);
    TEST_ASSERT_EQUAL_INT(16, required_out_size);

    ret = base64_encode(buffer, buffer_size, \
                            element_base64_out, &element_base64_out_size);

    TEST_ASSERT_EQUAL_INT(BASE64_SUCCESS, ret);
    TEST_ASSERT_EQUAL_INT(expected_out_size, element_base64_out_size);
}

So any consensus or BCP to solve this?

No BCP AFAIK. But enforcing defines for every test value seems rather non-sensical to me. One of the strongest proponents against literals is @OlegHahm as far as I experienced it, maybe he has some opinion on that?

OlegHahm · 2017-01-16T17:50:13Z

I'm fine with the enum solution, but would prefer to have it documented since it's not very common.

Kijewski · 2017-01-17T15:08:58Z

I'm fine with the enum solution, but would prefer to have it documented since it's not very common.

In my experience it's a very common ideom, because C does not know C++'s constexpr specifier, and #defines are quite likely to cause name clashes. I don't really know what the documentation should say here.

OlegHahm · 2017-01-17T15:17:17Z

In my experience it's a very common ideom, because C does not know C++'s constexpr specifier, and #defines are quite likely to cause name clashes.

Maybe in projects with poor naming conventions... ;-b

I don't really know what the documentation should say here.

Maybe the reason why OSX would fail for former solution.

OlegHahm · 2017-01-17T15:20:43Z

For the record, I don't object merging as is, but if you could think of a nice comment, I would be even happier.

smlng · 2017-01-18T12:36:51Z

I've to admit, the current solution is IMO sub optimal, too, because

though it works and fixes the issue, its an uncommon code construct for most, as it seems
its inconsistent with the rest of the code

I would go for size_t bufsize = 16, but that clashes for i < bufsize. So any thoughts, or leave as is to get macOS working and refine later?

OlegHahm · 2017-01-18T12:42:12Z

So any thoughts, or leave as is to get macOS working and refine later?

Fine with me.

smlng · 2017-01-18T12:48:21Z

added comment as suggested

miri64

Let's merge this

smlng mentioned this pull request Jan 12, 2017

tests: fix linker failure on OS X due to name clash #6326

Merged

Kijewski suggested changes Jan 12, 2017

View reviewed changes

Kijewski reviewed Jan 12, 2017

View reviewed changes

smlng force-pushed the pr/unittests/osx_array_fix branch from 2d58203 to 0796dc3 Compare January 12, 2017 19:57

smlng mentioned this pull request Jan 12, 2017

jenkins: add initial config #6258

Merged

smlng force-pushed the pr/unittests/osx_array_fix branch 2 times, most recently from 441da2d to 3f122cc Compare January 13, 2017 08:01

smlng mentioned this pull request Jan 13, 2017

CI Jenkins: activate macOS #6341

Closed

smlng force-pushed the pr/unittests/osx_array_fix branch from 3f122cc to b6aace4 Compare January 13, 2017 08:57

OlegHahm added OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system Area: tests Area: tests and testing framework labels Jan 13, 2017

smlng added this to the Release 2017.01 milestone Jan 13, 2017

miri64 reviewed Jan 14, 2017

View reviewed changes

smlng mentioned this pull request Jan 15, 2017

pkg/ccn-lite: add patch for compiler error #6352

Closed

BytesGalore reviewed Jan 16, 2017

View reviewed changes

miri64 assigned Kijewski and OlegHahm Jan 17, 2017

Kijewski approved these changes Jan 17, 2017

View reviewed changes

smlng force-pushed the pr/unittests/osx_array_fix branch from b6aace4 to 07953f3 Compare January 18, 2017 12:48

unittests: fix array length error on OSX

48a93cb

smlng force-pushed the pr/unittests/osx_array_fix branch from 07953f3 to 48a93cb Compare January 18, 2017 12:51

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 18, 2017

miri64 approved these changes Jan 18, 2017

View reviewed changes

miri64 merged commit 53ab146 into RIOT-OS:master Jan 18, 2017

smlng deleted the pr/unittests/osx_array_fix branch May 31, 2017 18:03

Conversation

smlng commented Jan 12, 2017

Uh oh!

Kijewski Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

smlng Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

Kijewski Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng Jan 13, 2017

Choose a reason for hiding this comment

Uh oh!

smlng commented Jan 13, 2017

Uh oh!

miri64 Jan 14, 2017

Choose a reason for hiding this comment

Uh oh!

BytesGalore Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

BytesGalore Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

OlegHahm Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

smlng commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

smlng commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

OlegHahm commented Jan 16, 2017

Uh oh!

Kijewski commented Jan 17, 2017

Uh oh!

OlegHahm commented Jan 17, 2017

Uh oh!

OlegHahm commented Jan 17, 2017

Uh oh!

smlng commented Jan 18, 2017

Uh oh!

OlegHahm commented Jan 18, 2017

Uh oh!

smlng commented Jan 18, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Kijewski Jan 12, 2017 •

edited

Loading

OlegHahm Jan 16, 2017 •

edited

Loading

smlng commented Jan 16, 2017 •

edited

Loading