Skip to content

Conversation

@Gizzzzmo
Copy link

To resolve #203: When the CMake cache variables is enabled / when the equivalent define is set, the main header only exposes fctprintf and vfctprintf.

Only when the CMake cache variable is set are all other functions (those that depend on putchar_) not compiled into the library. This is to stay consistent with the way other options are currently handled, although I feel like this might be a design flaw, and it would be better to have printf.h unconditionally include the config header, which then would also need to be installed.

To separate the functionality cleanly, and to allow linking with the entire (static) library even when it wasn't built with the new flag without having to define putchar_, I split the functionality into two translation units. All code shared between the two TUs is in a new header internal.h.

This begs another question: Should I move the main header emtest.h into a separate include directory, and make only that one publicly accessible with target_include_directories?
Right now if a consumer of the libary adds the project as a subdirectory, then linking with the target allows them to include "printf/internal.h" which
(a) is probably undesirable, and
(b) when installing the library the header is not installed, so it can't be included

Incidentally this was already possible with the source file printf.c. Although I doubt any user will have done #include <printf/printf.c>, it is technically a backwards-compatibility problem.

@eyalroz
Copy link
Owner

eyalroz commented Nov 23, 2025

This is a v-e-r-y extensive change, which I am not inclined to accept. Why do you believe it's not sufficient, to address the issue in #203, to support suppressing printf_(), vprintf_() and putchar(), and be done with it?

When disabled, the main header doesn't expose printf_ and vprintf_,
and these functins are not compiled into the library.
Code is split into separate translation units for this purpose,
and to allow linking with the full (static) library without supplying a definition for putchar_.
@Gizzzzmo
Copy link
Author

As I said in the issue discussion I find it more comfortable to use this way. Specifically what I mean are two scenarios:

  1. With this approach I am able to build and install the (static) library, once, without disabling the "plain printf" feature (tbf I do like that name better than what I came up with), and link it both to a binary that uses the plain printf_ function, and one that doesn't without having to provide a putchar_ definition for the latter
  2. Even when not installing, if I have e.g. a CMake project that adds printf as a subdirectory and I have two executable targets, where again one uses the plain printf, and the other doesn't, then I can't disable it just for one of the two.

Regarding the extent of the changes:
It looks like more than it actually is because I renamed printf.c to printf_core.c, and added a new file printf.c. I thought the names made sense, but If I just rename things, these are the diff stats I get:

 CMakeLists.txt        |  10 ++++++--
README.md             |   3 ++-
printf_config.h.in    |   1 +
src/printf/internal.h |  71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/printf/printf.c   | 113 +++++-------------------------------------------------------------------------------------
src/printf/printf.h   |   7 ++++++
src/printf/printf_.c  |  34 +++++++++++++++++++++++++++
test/test_suite.cpp   |   1 +
8 files changed, 130 insertions(+), 110 deletions(-)

And all the ~100 deletions in printf.c are just cut-and-pastes into the new files, meaning other than the file restructuring there are only about 30 new lines of code. Sure the restructuring itself isn't nothing, but it doesn't intrdocue as many changes as the diff stat would suggest.

While reading through your PR I also realized that I forgot about the [v]s[n]printf_ not requiring putchar_, but that should be fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants