xiaoxiang781216 commented on PR #16544: URL: https://github.com/apache/nuttx/pull/16544#issuecomment-2984419611
> Thank you @xiaoxiang781216, sorry but I have questions :-P > > * Please update description WHY this change is necessary and WHAT IT FIXES. I see no benefit, no additional functionality, except function names change, The change make the internal api more consistent with the standard api defined in stdio.h > and potential self-compatibility break (we change both nuttx and nuttx-apps and CI reveals build errors). > The api is nuttx specific, seldom used in apps(4 places) and trivial to fix, so I think it's better to improve the consistence. > * Is this change bringing us closer to ANSI/POSIX standards or diverge from the standards? no impact, since all stream base function is for internal use. > > * Even if for now the `lib_outstream_s *` output functions are implemented, maybe one day string or file implmentations will show up, and this keeps naming API aligned with ANSI/POSIX? no stream is a general concept, nuttx already implement many: memory, stdio, fd, file, syslog, block, mtd, null, lzf, base64, hex and buffered. even file backend still call lib_printf, not lib_fprintf. > > * Does it help you in porting some software components? Why this change is necessary? > no change here. > * For instance you want to rename `lib_sprintf` to `lib_printf` but still the first argument is the output stream so we break printf's API [1] because `printf()` writes output `stdout` and its first argument is not the output stream but format. > > * Do I get this wrong? > printf assume the output always goto stdout, that's why it omits the destination argument from function prototype. So, lib_printf should be compared with fprintf, sprintf and dprintf, not printf. since stream is a general concept, one lib_printf could support to output to FILE, string and fd by pass the different stream backend(e.g. lib_stdoutstream_s, lib_memoutstream_s and lib_rawsostream_s). > > * There are three changes bundled into this single PR: 1. `lib_scanf()` limplementation, 2. `lib_bsprintf()` updates, 3. `printf` API renames in many places while the PR title and secription does not mention that and may be misleading. My objections are to (3 / [d241c7a](https://github.com/apache/nuttx/commit/d241c7a802f17437af1e162d8f9636e2e4c578da)) while (1) and (2) seems okay, maybe we should split this PR and move (1) and (2) to a separate PR? > Ok, let's update PR. -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
