You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/20 19:53:25 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #5296: tcp: fixed ISO C89/C90 related warnings
a-lunev opened a new pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296
## Summary
Fixed the following types of warnings:
warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘struct tcp_conn_s *’ [-Wformat=]
warning: ISO C90 does not support the ‘z’ gnu_printf length modifier [-Wformat=]
warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
## Impact
TCP
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789638749
##########
File path: net/tcp/tcp_accept.c
##########
@@ -232,7 +232,7 @@ int psock_tcp_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
{
/* Yes... get the address of the connected client */
- ninfo("Pending conn=%p\n", state.acpt_newconn);
+ ninfo("Pending conn=%p\n", (void *)state.acpt_newconn);
Review comment:
%p for the non void pointer is used in many many place, and the usage is harmless too. The compiler warning is stupid sometime:(.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789395677
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -191,10 +191,10 @@ static inline uint16_t tcp_newdata(FAR struct net_driver_s *dev,
if (nsaved < buflen)
{
nwarn("WARNING: packet data not fully saved "
- "(%d/%u/%zu/%u bytes)\n",
+ "(%d/%u/%u/%u bytes)\n",
buflen - nsaved,
(unsigned int)nsaved,
- recvlen,
+ (unsigned int)recvlen,
Review comment:
can we relax the %z flag? the library feature is different from language(compiler) feature. libc is provided by NuttX self, and we actually support many c99 feature. So, c89 in NuttX should be the core/compiler feature, not the library feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789663023
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Yes, you are right. The last issue will become error in the old compiler. But, the first two aren't.
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Yes, you are right. The last issue will become an error in the old compiler. But, the first two aren't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789618460
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
NuttX defines stdint related types by itself:
nuttx/include/stdint.h:
`typedef _uint32_t uint32_t;
`
nuttx/arch/z80/include/z80/types.h:
`typedef unsigned long _uint32_t;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed warning: ISO C90 forbids mixed declarations and code
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789709201
##########
File path: net/tcp/tcp_accept.c
##########
@@ -232,7 +232,7 @@ int psock_tcp_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
{
/* Yes... get the address of the connected client */
- ninfo("Pending conn=%p\n", state.acpt_newconn);
+ ninfo("Pending conn=%p\n", (void *)state.acpt_newconn);
Review comment:
I have updated the patch. ("warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘struct tcp_conn_s *’ [-Wformat=]" is not required to fix).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789465654
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -191,10 +191,10 @@ static inline uint16_t tcp_newdata(FAR struct net_driver_s *dev,
if (nsaved < buflen)
{
nwarn("WARNING: packet data not fully saved "
- "(%d/%u/%zu/%u bytes)\n",
+ "(%d/%u/%u/%u bytes)\n",
buflen - nsaved,
(unsigned int)nsaved,
- recvlen,
+ (unsigned int)recvlen,
Review comment:
I do not see any explicit way to disable `%z` warning while keeping `-Wformat` on. But I agree that since NuttX supports it's own implementation of `printf` we should relax `%z` check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789637564
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
I suggest that the real change we need is move the variable definition in the middle to the begin, other change could be safely ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789636114
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
> Yes, that's why I suggest that we only need to conform c89 core language spec, not the whole c89 spec. We have to stick to c89 just because some arch compiler can't support the c99 grammar. From the language perspective, c99 is much better than c89, so we should leverage the new 99 feature which not relate to the grammar(such as the platform independent type) as much as possible.
I agree. So what is the issue with the highlighted line (uint32_t sndlen)? What is your suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789628858
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Yes, that's why I suggest that we only need to conform c89 core language spec, not the whole c89 spec. We have to stick to c89 just because some arch compiler can't support the c99 grammar. From the language perspective, c99 is much better than c89, so we should leverage the new 99 feature which not relate to the grammar(such as the platform independent type).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789395677
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -191,10 +191,10 @@ static inline uint16_t tcp_newdata(FAR struct net_driver_s *dev,
if (nsaved < buflen)
{
nwarn("WARNING: packet data not fully saved "
- "(%d/%u/%zu/%u bytes)\n",
+ "(%d/%u/%u/%u bytes)\n",
buflen - nsaved,
(unsigned int)nsaved,
- recvlen,
+ (unsigned int)recvlen,
Review comment:
can we relax the %z flag? the library feature is different from language(compiler) feature. libc is provided by NuttX self, and we actually support many c99 feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789637583
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
I think here just was a highlight to say that `%z` follows the similar case as stdint types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789703277
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Good. I have updated the patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789628858
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Yes, that's why I suggest that we only need to conform c89 core language spec, not the whole c89 spec. We have to stick to c89 just because some arch compiler can't support the c99 grammar. From the language perspective, c99 is much better than c89, so we should leverage the new 99 feature which not relate to the grammar(such as the platform independent type) as much as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789640089
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Do you mean we should not care about these two types of warnings?
warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘struct tcp_conn_s *’ [-Wformat=]
warning: ISO C90 does not support the ‘z’ gnu_printf length modifier [-Wformat=]
However, we should care only about the following warning type:
warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
Right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5296: tcp: fixed warning: ISO C90 forbids mixed declarations and code
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789396486
##########
File path: net/tcp/tcp_accept.c
##########
@@ -232,7 +232,7 @@ int psock_tcp_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
{
/* Yes... get the address of the connected client */
- ninfo("Pending conn=%p\n", state.acpt_newconn);
+ ninfo("Pending conn=%p\n", (void *)state.acpt_newconn);
Review comment:
Any type pointer can implicitly convert to void *, is it too redundant to add cast here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789396054
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
If we strictly follow C89, we can't even use uint32_t since it define in C99.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789592118
##########
File path: net/tcp/tcp_accept.c
##########
@@ -232,7 +232,7 @@ int psock_tcp_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
{
/* Yes... get the address of the connected client */
- ninfo("Pending conn=%p\n", state.acpt_newconn);
+ ninfo("Pending conn=%p\n", (void *)state.acpt_newconn);
Review comment:
If I compile the file by passing "-std=c89 -Wpedantic", the gcc produces "warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘struct tcp_conn_s *’ [-Wformat=]".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed warning: ISO C90 forbids mixed declarations and code
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789708381
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -191,10 +191,10 @@ static inline uint16_t tcp_newdata(FAR struct net_driver_s *dev,
if (nsaved < buflen)
{
nwarn("WARNING: packet data not fully saved "
- "(%d/%u/%zu/%u bytes)\n",
+ "(%d/%u/%u/%u bytes)\n",
buflen - nsaved,
(unsigned int)nsaved,
- recvlen,
+ (unsigned int)recvlen,
Review comment:
I have updated the patch. ("warning: ISO C90 does not support the ‘z’ gnu_printf length modifier [-Wformat=]" is not required to fix).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5296: tcp: fixed ISO C89/C90 related warnings
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5296:
URL: https://github.com/apache/incubator-nuttx/pull/5296#discussion_r789640089
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -274,6 +276,8 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+ uint32_t sndlen;
Review comment:
Do you mean we should do not care about these two types of warnings?
warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘struct tcp_conn_s *’ [-Wformat=]
warning: ISO C90 does not support the ‘z’ gnu_printf length modifier [-Wformat=]
However, we should care only about the following warning type:
warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
Right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org