[riot-notifications] [RIOT-OS/RIOT] gcoap: DEVELHELP checks resources are sorted (#7580)

Gaƫtan Harter notifications at github.com
Tue Sep 19 11:19:11 CEST 2017


cladmi commented on this pull request.



> @@ -570,6 +570,32 @@ static void _find_obs_memo_resource(gcoap_observe_memo_t **memo,
 }
 
 /*
+ * Check listener structure
+ *
+ *  - resources should be sorted alphabetically
+ */
+#ifdef DEVELHELP
+static void _check_listener(gcoap_listener_t *listener)

True, and even more as I put a name with `check` in it.

> @@ -570,6 +570,32 @@ static void _find_obs_memo_resource(gcoap_observe_memo_t **memo,
 }
 
 /*
+ * Check listener structure
+ *
+ *  - resources should be sorted alphabetically
+ */
+#ifdef DEVELHELP
+static void _check_listener(gcoap_listener_t *listener)
+{
+    const char *prev_path = "";
+    const char *path = NULL;
+
+    for (size_t i = 0; i < listener->resources_len; i++) {

This is micro-optimisation on helper code :) I was focusing on the idea not the implementation.
And does it really reduce code size ?

> @@ -594,6 +620,8 @@ kernel_pid_t gcoap_init(void)
 
 void gcoap_register_listener(gcoap_listener_t *listener)
 {
+    _check_listener(listener);

I made it without assert as I wanted to make this non crashing.
In fact it could still work with a non-sorted list if the resources are sorted per request type.
For me, the choice of enforcing it is under the `gcoap` maintainer responsibility.

> @@ -806,7 +834,7 @@ uint8_t gcoap_op_state(void)
 int gcoap_get_resource_list(void *buf, size_t maxlen, uint8_t cf)
 {
     assert(cf == COAP_CT_LINK_FORMAT);
-#ifndef DEVELHELP
+#ifdef NDEBUG

The definition of `assert` depends on the definition of `NDEBUG`
`NDEBUG` is defined by the build system if `DEVELHELP`is defined with `-DDEVELHELP`.
https://github.com/RIOT-OS/RIOT/blob/2017.07/makefiles/cflags.inc.mk#L55

So this `ifdef` worked because of the build system, and if you do `CFLAGS += -DDEVELHELP=1`, `assert` is not defined and you get an unused variable warning.

It may belong in another PR though.

-- 
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/7580#discussion_r139631391
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20170919/0b0cf39d/attachment.html>


More information about the notifications mailing list