[riot-notifications] [RIOT-OS/RIOT] WIP net/tcp: make header option pointer c++ compliant (#11300)

Simon Brummer notifications at github.com
Thu Mar 28 07:10:17 CET 2019


brummer-simon requested changes on this pull request.

I am fine with replacing the dynamic sized array in general, but I wonder since then this is not iso-c++ compliant?


>  } tcp_hdr_opt_t;
 
+/**
+ * @brief   Get the start of the TCP option value
+ *
+ * @param[in]   opt   TCP option field helper structure contiguous memory
+ *
+ * @returns     pointer to first byte of the value
+ */
+inline static uint8_t *tcp_hdr_opt_value_ptr(tcp_hdr_opt_t *opt){

Why is this static? I don't see any point in having a static function outside of a compilation unit.

> @@ -55,7 +55,7 @@ int _option_parse(gnrc_tcp_tcb_t *tcb, tcp_hdr_t *hdr)
                     DEBUG("gnrc_tcp_option.c : _option_parse() : invalid MSS Option length.\n");
                     return -1;
                 }
-                tcb->mss = (option->value[0] << 8) | option->value[1];
+                tcb->mss = (tcp_hdr_opt_value_ptr(option)[0] << 8) | tcp_hdr_opt_value_ptr(option)[1];

I don't like find the style of "func()[0]" very readable and the calculation is redundant. Please store the Pointer in a variable and use it. I think I would be much clearer to read.

-- 
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/11300#pullrequestreview-219845010
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190327/7e5093e6/attachment-0001.html>


More information about the notifications mailing list