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 2020/06/02 17:06:31 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1179: libc: Implement vscanf() function
xiaoxiang781216 opened a new pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
Change-Id: I2421e57b2c848c43afb749b8ebfa79ec457cde26
## Summary
## Impact
## 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434112939
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
It would be much better to use vdprintf()
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434186409
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
```suggestion
return vdprintf(1, fmt, &ap);
```
This is the perfect replacement!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434238346
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
If you want that kind of nonstandard behavior from printf for some reason. You probably could create a configuration option to redirect print to syslog. But I think it is unacceptable as default behavior for printf() because it breaks it required behavior.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434113844
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
vdprintf() will behave exactly like printf, but will not depend on the FILE stdout. That is a better choice.
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
syslog() has other properties and may not output of fd == 0 as is REQUIRED for printf().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434114709
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
syslog() has other properties and may not output of fd == 1 as is REQUIRED for printf().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434570228
##########
File path: include/stdio.h
##########
@@ -192,10 +192,11 @@ int vfprintf(FAR FILE *stream, FAR const IPTR char *fmt,
va_list ap);
int vfscanf(FAR FILE *stream, FAR const IPTR char *fmt, va_list ap);
int vprintf(FAR const IPTR char *fmt, va_list ap);
+int vscanf(FAR const IPTR char *fmt, va_list ap);
Review comment:
These changes seem out of place in the PR. Can we move this to a different PR. We should not make any IPTR-related changes until we have a clear understanding and a plan for how we want to handle these.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434286064
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
Done.
Yes, vdprintf is better. The code is blindly copied from printf.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434700105
##########
File path: libs/libc/stdio/lib_printf.c
##########
@@ -59,11 +55,7 @@ int printf(FAR const IPTR char *fmt, ...)
int ret;
va_start(ap, fmt);
-#if CONFIG_NFILE_STREAMS > 0
- ret = vfprintf(stdout, fmt, ap);
-#else
- ret = nx_vsyslog(LOG_INFO, fmt, &ap);
-#endif
+ ret = vdprintf(STDOUT_FILENO, fmt, ap);
Review comment:
I already merged this, but it is not correct. printf must used streams and buffere C I/O if available.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434744776
##########
File path: libs/libc/stdio/lib_printf.c
##########
@@ -59,11 +55,7 @@ int printf(FAR const IPTR char *fmt, ...)
int ret;
va_start(ap, fmt);
-#if CONFIG_NFILE_STREAMS > 0
- ret = vfprintf(stdout, fmt, ap);
-#else
- ret = nx_vsyslog(LOG_INFO, fmt, &ap);
-#endif
+ ret = vdprintf(STDOUT_FILENO, fmt, ap);
Review comment:
Yes, you are right. I forget the I/O buffer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179#discussion_r434286064
##########
File path: libs/libc/stdio/lib_vprintf.c
##########
@@ -49,7 +52,13 @@
int vprintf(FAR const IPTR char *fmt, va_list ap)
{
+#if CONFIG_NFILE_STREAMS > 0
/* vfprintf into stdout */
return vfprintf(stdout, fmt, ap);
+#else
+ /* vsyslog into debug channel */
+
+ return nx_vsyslog(LOG_INFO, fmt, &ap);
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo merged pull request #1179: libc: Implement vscanf() function
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #1179:
URL: https://github.com/apache/incubator-nuttx/pull/1179
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org